-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Add disallow_deprecation_warnings:rails #16
Conversation
Co-authored-by: Étienne Barrié <etienne.barrie@shopify.com>
8f69f35
to
95750f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this would be a nice feature! 😃
I like that the implementation is compact, but from an architectural standpoint, it feels a little odd to handle this from inside the deprecator. What I mean is, the way I would accomplish this without any special API would be something like:
Rails.application.deprecators.each do |deprecator|
deprecator.disallowed_warnings = :all if deprecator.gem_name == "Rails"
end
To bridge the gap between that and config.active_support.disallowed_deprecation_warnings = :rails
, perhaps we could introduce a Deprecators#disallow_warnings_from
method:
# Could also accept a pattern (e.g. `/rails/i`) or multiple args
Rails.application.deprecators.disallow_warnings_from("Rails")
Then we could potentially add a config.active_support.disallow_warnings_from
config setting. Though there might be a better / more consistent name.
Thoughts?
…lude the options of `:all` and `:rails`
Or another possibility would be to add a # Could also accept a pattern (e.g. `/rails/i`)
Rails.application.deprecators.disallow_warnings(gem: "Rails") Afterward, we could add a matching option to Rails.application.deprecators.silence(gem: "Rails") do
# ...
end |
👋🏻 appreciate the fast feedback! Fair point about the architectural awkwardness, I like the second option and will explore it asap! |
Oh yeah that would totally work and require no additional API. Given rails#46550 we should be able to do that in |
Will close for now as we will go with that no-api approach. Will revisit in the future if there is a need for other gems to be deprecated! |
What are you trying to accomplish?
This work is a follow up of deprecation work handling the new
Rails.application.deprecation
for our Rails Upgrade script. We discussed that it would be nice to have a way to only configure the deprecations from Rails within an application as its those (Rails) deprecations that would need to be actioned on during a Rails upgrade.Example of this working with the following test:
1 deprecation from Rails, 1 deprecation defined in the sandbox itself- notice how only the deprecation from rails is "raised" as an error.
![image](https://user-images.githubusercontent.com/20158582/231295531-1f12b9a6-5f3c-4113-9440-3da703aa013b.png)
Work remaining:
config.active_support_disallowed_deprecation_warnings
section to include mention of:all
and:rails
What should reviewers focus on?
Before you deploy