search

Shopify / deprecation_toolkit

⚒Eliminate deprecations from your codebase ⚒
MIT License
454 stars 40 forks source link

Deprecation toolkit itself using deprecated code? #88

Closed gstokkink closed 8 months ago

gstokkink commented 9 months ago

Hi,

When I use the deprecation-toolkit gem, and it issues a warning, I get the following extra warning from Rails:

DEPRECATION WARNING: Calling warn on ActiveSupport::Deprecation is deprecated and will be removed from Rails (use your own Deprecation object instead) (called from warn at /Users/work/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-7.1.1/lib/active_support/deprecation/instance_delegator.rb:55)

Looks like the following code may need to be patched to use a locally stored ActiveSupport::Deprecation instance rather than the ActiveSupport::Deprecation singleton: https://github.com/Shopify/deprecation_toolkit/blob/main/lib/deprecation_toolkit/warning.rb#L53

This also goes for the code here: https://github.com/Shopify/deprecation_toolkit/blob/main/lib/deprecation_toolkit.rb#L48.

Refer to https://github.com/rails/rails/pull/47354.

I'm using Rails 7.1.1. and deprecation-toolkit 2.0.3.

gstokkink commented 8 months ago

@davidstosik just wondering, is this project still active? I noticed that an issue from May had yet to receive a reply.

gstokkink commented 7 months ago

@etiennebarrie thanks for the PR, but it seems like there are still some instances of deprecated code usage left unfortunately :(

DEPRECATION WARNING: Calling behavior on ActiveSupport::Deprecation is deprecated and will be removed from Rails (use Rails.application.deprecators[framework].behavior where framework is for example :active_record instead) (called from block in add_notify_behavior at /Users/work/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/deprecation_toolkit-2.0.4/lib/deprecation_toolkit.rb:26)
DEPRECATION WARNING: Calling behavior= on ActiveSupport::Deprecation is deprecated and will be removed from Rails (use Rails.application.deprecators.behavior= instead) (called from block in add_notify_behavior at /Users/work/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/deprecation_toolkit-2.0.4/lib/deprecation_toolkit.rb:29)

I think it's because here the ActiveSupport::Deprecation singleton is yielded as fallback:

https://github.com/Shopify/deprecation_toolkit/blob/main/lib/deprecation_toolkit.rb#L48

Obviously, this will only matter in non-Rails applications (in this case, a gem).

etiennebarrie commented 7 months ago

Indeed, yes this gem was mostly meant to be used by Rails applications directly, as evidenced by the readme. But there's not much preventing it from being used by a gem. Right now both the Minitest and RSpec plugin call DeprecationToolkit.add_notify_behavior which looks for Rails application deprecators, but I feel like we could add a list of deprecators to the configuration, and derive the attach_to from the deprecators.

So a gem could add its dependencies' ActiveSupport::Deprecation instances in the configuration, and then when the test plugin kicks in, it would add the notify behavior to them, and it would attach to the correct name automatically, using the Deprecation's gem_name, since that's what the :notify behavior does.

Would you be interested in contributing this? Do you have an example public gem using it?