Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deprecate ActiveSupport::Deprecation singleton usage #47354
Deprecate ActiveSupport::Deprecation singleton usage #47354
Changes from all commits
675cf79
6ce3dd9
8171ee0
83df54c
8f6eda6
33c5a60
e5af9c2
0f0aa86
c8eeac5
a3061e9
a7feaaf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For the prose parts, let's keep the method doc wrapped at 80-ish characters.
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.
It's not a blocker, but keeping lines like this (not code examples) wrapped at 80-ish characters would be consistent with other method docs (for example, further down).
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.
If we could go back in time, I would rename
ActiveSupport::Deprecation
toActiveSupport::Deprecator
, but it may not worth the effort now. 🤷♂️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.
Could we give an example here? Consider this code in an app (not in a gem/engine), which I think is pretty common and there is lots of internet recommendations to use:
You get this warning:
I think a more useful warning would be:
Unless we are recommending that apps define their own deprecators as well? (Which to me seems excessive for most apps.)
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.
#47978
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 that we could give a better example of how to fix this deprecation. We absolutely do not want to point to
ActiveSupport.deprecator
though. The whole point of this deprecation cycle is to stop third-party code from generating deprecations that seem to come from Rails.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.
But what about first party code? What should it do?
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 meant third-party from the point of view of Rails, i.e. libraries and the application.
Regarding application deprecations, at Shopify, we've added an application deprecator in
config/application.rb
:And then we use it directly:
We've debated whether to do like a gem would typically do and have an API like
Shopify.deprecator
but ultimately felt like the out-of-the-box API from Rails was good enough.I also thought that maybe Rails could always define an application deprecator to be used directly but didn't propose it because ultimately I don't think most apps need deprecations (it's possible to just change callers and callees at once, unlike in a library/framework), the deprecation horizon doesn't make much sense, etc.
If an application needs a deprecator, it can do what we did above, or do like in a gem, define a singleton method on the application module, set in the application deprecators in an initializer.
I'm not sure what we should write in the deprecation to make that clearer, there's some setup involved… Yes the deprecated API was clean and nice, but in the end it was misguided as separate gems and applications in addition to Rails were all sharing the same
Deprecation
instance.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.
Hmm. I don't think the fact that setup is required should be celebrated. I understand Shopify doing this but for the long tail of small apps I think this is overly complicated vs. what Rails 7 and before offered.
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.
Similarly, to cover e.g.
ActiveSupport::Deprecation.instance.behavior = ...
:Or, if we make
ActiveSupport.deprecator
==ActiveSupport::Deprecation._instance
: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.
This one is not necessary/possible, as calling
ActiveSupport::Deprecation.instance
will first emit a deprecation, before we ever have a chance to set the behavior on it, whether we emit the deprecation before calling super or not.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.
You're right — swapping the order here is nonsensical. 🤦♂️ 🤦♂️
Using
ActiveSupport::Deprecation._instance
as the underlying deprecator will benefit subsequent calls though. e.g.:(Which is now covered since
ActiveSupport.deprecator
isActiveSupport::Deprecation._instance
.)