Skip to content

Add category to rb_warn_deprecated - #3505

Merged
jeremyevans merged 1 commit into
ruby:masterfrom
eileencodes:followup-on-category-warning
Sep 2, 2020
Merged

Add category to rb_warn_deprecated#3505
jeremyevans merged 1 commit into
ruby:masterfrom
eileencodes:followup-on-category-warning

Conversation

@eileencodes

Copy link
Copy Markdown
Member

PR #3418 added a category to
rb_warn_deprecated_to_remove but not to rb_warn_deprecated. This
adds the same code to rb_warn_deprecated so that those warnings also
get a category.

This change also adds tests for rb_warn_deprecated and updates the
tests for rb_warn_deprecated_to_remove to have clearer names.

Feature: https://bugs.ruby-lang.org/issues/17122

@jeremyevans jeremyevans left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think it results in some duplication, but that can be refactored later.

@jeremyevans

Copy link
Copy Markdown
Contributor

I found the bug that causes the spec failures. You need to use: me = rb_method_entry(rb_singleton_class(rb_mWarning), id_warn);, since you are calling the singleton method, not the instance method. Both in rb_warn_deprecated and rb_warn_deprecated_to_remove. I'm guessing the specs override only the singleton method and not the instance method.

PR ruby#3418 added a category to
`rb_warn_deprecated_to_remove` but not to `rb_warn_deprecated`. This
adds the same code to `rb_warn_deprecated` so that those warnings also
get a category.

This change also adds tests for `rb_warn_deprecated` and updates the
tests for `rb_warn_deprecated_to_remove` to have clearer names.

I've fixed the call to `rb_method_entry` as we need to be using the
instance method, not singleton.

Feature: https://bugs.ruby-lang.org/issues/17122
@eileencodes
eileencodes force-pushed the followup-on-category-warning branch from 3a9ceb3 to f099959 Compare September 2, 2020 18:58
@jeremyevans
jeremyevans merged commit eada635 into ruby:master Sep 2, 2020
@eileencodes
eileencodes deleted the followup-on-category-warning branch September 2, 2020 20:40
@eileencodes

Copy link
Copy Markdown
Member Author

I wasn't able to get around to looking at refactoring this a bit to be less repetitive. Will try to get to it tomorrow.

@jeremyevans

Copy link
Copy Markdown
Contributor

I've already started working on changes locally based on this PR that include such a refactoring, so there is no need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants