-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Improve docs of the load_defaults
method [ci skip]
#37595
Improve docs of the load_defaults
method [ci skip]
#37595
Conversation
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.
Good idea 👍 . Reviewed multiple PR where original author thought it would load only the version specified
guides/source/configuring.md
Outdated
@@ -926,6 +926,8 @@ text/javascript image/svg+xml application/postscript application/x-shockwave-fla | |||
- `config.active_storage.replace_on_assign_to_many`: `true` | |||
- `config.active_record.collection_cache_versioning`: `true` | |||
|
|||
Note that the method not only does load defaults of a version you specified, but it also loads default configurations of all previous versions. |
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 think mapping the "With 'x.x'" with this text is too cumbersome, I've been wondering if we can rephrase the headers to communicate that it's <= 6.0 for instance.
Maybe if we sort them in descending order and say something like "With '6.0', these including previous versions' new defaults"?
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.
Yeah, sorting them in descending and rephrasing them to emphasize that "it includes previous versions' new defaults" will definitely be easier to understand. Thanks for the advice.
It would be great to backport to 6-0-stable
to refresh this on https://guides.rubyonrails.org when a new version of 6.0.x is released?
6bbe70e
to
5014061
Compare
I changed `load_defaults` to `config.load_defaults` in the guide to emphasize its context, sorted headers "With 'x.x'" in descending and rephrased them to "With 'x.x', it includes previous versions' new defaults" and documented the method on https://api.rubyonrails.org.
5014061
to
18b2dd2
Compare
Thanks @bogdanvlviv! Realized how tough this really is to explain and tried some versions too with some explaining text up top. Think we ended up in a much clearer place than before, thanks for raising this 🙏 Also back ported to 6-0-stable. |
Thank you for the review! |
I changed
load_defaults
toconfig.load_defaults
in the guideto emphasize its context,
sorted headers "With 'x.x'" in descending and rephrased them
to "With 'x.x', it includes previous versions' new defaults"
and documented the method on https://api.rubyonrails.org.