Project

General

Profile

Feature #17143

Improve support for warning categories

Added by jeremyevans0 (Jeremy Evans) about 2 months ago. Updated 24 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:99856]

Description

Support was recently added for Warning.warn to accept a category keyword. However, the initial implementation was limited to having rb_warn_deprecated and rb_warn_deprecated_to_remove use the :deprecated value for the category keyword.

It doesn't make sense to me to have a category keyword if it is only used for deprecation, so I propose we extend the support so that Kernel#warn accepts a category keyword (for Ruby-level warnings) and rb_category_warn and rb_category_warning functions be added to the C-API (for C-level warnings). I also propose that we change existing rb_warn and rb_warning calls to rb_category_warn and rb_category_warning, so that all warnings issued by core Ruby are issued with an appropriate category.

I have implemented support for this in a pull request: https://github.com/ruby/ruby/pull/3508

Updated by ko1 (Koichi Sasada) about 1 month ago

Let me clear the proposal. Your PR seems to allow any categories (and any class of object). Is it intentional?

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

ko1 (Koichi Sasada) wrote in #note-1:

Let me clear the proposal. Your PR seems to allow any categories (and any class of object). Is it intentional?

Allowing arbitrary categories is intentional. We probably should restrict the category to be a symbol. The C-API already does this (rb_category_warn{,ing} accept const char * and convert to symbol), so it would only need to be made for Kernel#warn (raising TypeError if :category is not a Symbol?). That's a simple change, and I'll try to make it tomorrow, before the developer meeting.

I'm fine with changing the implementation to limit the categories to specific categories and not allow arbitrary categories, if that is what is decided at the developer meeting. That reduces flexibility, but also reduces the chance of a typo resulting in a category being missed. Allowing arbitrary categories has the advantage that users can define their own categories of warnings and filter on them.

#3

Updated by ko1 (Koichi Sasada) about 1 month ago

My concern is, arbitrary categories make filtering difficult on Warning.warn method.

Another concern is, "what is the category?" problem.
You added "file" category, but some warning should be "deprecated".
These two categories can be combine (file and deprecated).
Allow multiple categories?

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

ko1 (Koichi Sasada) wrote in #note-3:

My concern is, arbitrary categories make filtering difficult on Warning.warn method.

Agreed. It's definitely a tradeoff. As I said, I'm fine disallowing arbitrary categories and will make the necessary changes if that is what is decided at the developer meeting.

Another concern is, "what is the category?" problem.
You added "file" category, but some warning should be "deprecated".
These two categories can be combine (file and deprecated).
Allow multiple categories?

I don't think we should allow multiple categories in a single warning. I'm fine changing any categories I used or combining any of the categories I used into an existing category (e.g. file -> deprecated), and will make the necessary changes if that is what is decided at the developer meeting.

Updated by matz (Yukihiro Matsumoto) 30 days ago

Adding category to the warning seems a good idea. But I have the following concerns:

  • Category should be specified only by symbols
  • Category symbols must be among predefined set (to avoid typos and confusions)

Matz.

Updated by shyouhei (Shyouhei Urabe) 30 days ago

  • I'm not against adding the category keyword. However,
  • I guess it is quite hard for us to +1 the "Add categories for all core warnings" commit at once.
    • It might contain OK changes, but might also contain questionable ones.
    • Do you really need to do this in a big changeset like this? Can you split it into each categories, and let us consider case-by-case?

Updated by jeremyevans0 (Jeremy Evans) 29 days ago

matz (Yukihiro Matsumoto) wrote in #note-5:

Adding category to the warning seems a good idea. But I have the following concerns:

  • Category should be specified only by symbols

OK, this change has already been made.

  • Category symbols must be among predefined set (to avoid typos and confusions)

I'll make this change. I'll start with only :deprecated being allowed, since that is already used. Future commits can add support for other symbols.

shyouhei (Shyouhei Urabe) wrote in #note-6:

  • I'm not against adding the category keyword. However,
  • I guess it is quite hard for us to +1 the "Add categories for all core warnings" commit at once.
    • It might contain OK changes, but might also contain questionable ones.
    • Do you really need to do this in a big changeset like this? Can you split it into each categories, and let us consider case-by-case?

I can split the commit into separate commits per category. I'll focus on the categories I think are most important first and submit separate pull requests for those.

Updated by jeremyevans0 (Jeremy Evans) 29 days ago

I've updated my pull request to only allow specific categories, and limit the currently allowed categories to :deprecated. Assuming it passes CI, I'll merge it, and then work on pull requests for additional categories.

Updated by jeremyevans0 (Jeremy Evans) 24 days ago

I submitted the first pull request to add the :deprecated category to additional warnings, and to add warning categories for :uninitialized_ivar and :redefine: https://github.com/ruby/ruby/pull/3601

matz (Yukihiro Matsumoto) Do you want to approve all warning categories before they are merged, or are you comfortable delegating approval of warning categories to another developer (such as shyouhei (Shyouhei Urabe))? We can revert any warning category you are not comfortable with, as a separate commit will be used for each category.

Also available in: Atom PDF