-
Notifications
You must be signed in to change notification settings - Fork 486
Migrate strip_trailing_whitespace to use component-local config under the hood
#2230
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
Conversation
7caa69d to
3f1ba88
Compare
3f1ba88 to
cc16f0f
Compare
afa75ec to
44d4fc8
Compare
joelhawksley
left a comment
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 looks good. What docs updates do you think we should make?
|
I was thinking we should have docs for the new config ready against v4 instead, but now that I think about it, it'd make sense to at least reflect the deprecation in the current docs. I'll do that next and accept that tweak to the changelog message. |
2b17e09 to
0eb9744
Compare
trim_trailing_whitespace to use component-local config under the hoodstrip_trailing_whitespace to use component-local config under the hood
e330342 to
636fc43
Compare
joelhawksley
left a comment
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.
Looks good to me! Some minor notes.
98bd710 to
303f856
Compare
303f856 to
3fab950
Compare
What are you trying to accomplish?
Following an initial exploration of this in #2210, I agreed with @joelhawksley and @camertron that we should take an incremental approach and roll out component- and engine-local config in V3, making deprecations that would be effective come v4. Our end goal here is to make it more practical to include view_component as a dependency in engines and gems, which are currently subject to the same config as the parent app they're included into.
trim_trailing_whitespace seemed to be an ideal candidate for being part of a component-local config entity, since (being an instance variable already) it effectively works in the same way.
What approach did you choose and why?
The ComponentLocalConfig module essentially replicates ActiveSupport::Configurable, but puts the config on
configurationas opposed toconfig, which appears to point toRails.application.configas-is. This could be changed down the line, but since I wasn't sure of what thatconfigalias was used for right now, I essentially aliased the functionality. That having been said, we could always change this later if it turns out that it's not something that will cause much trouble - these two method names being similar could potentially be an issue when component-local config is more public.Anything you want to highlight for special attention from reviewers?
Because the component local config module essentially aliases functionality we'd otherwise get from
ActiveSupport::Configurable, it might be safe for us not to test it directly. It's minimally tested for inheritability in this PR. I had to mark a couple of methods asnocovsince I'm almost certain that they're exercised during init, but Simplecov didn't appear to pick them up.