-
Notifications
You must be signed in to change notification settings - Fork 22k
Clarify config settings for AS::halt_callback_chains_on_return_false #22709
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
Clarify config settings for AS::halt_callback_chains_on_return_false #22709
Conversation
r? @sgrif (@rails-bot has picked a reviewer for you, use r? to override) |
Can we just add docs for the setting instead? I don't think we need to exhaustively document config options in the config file itself. |
76308be
to
6d59427
Compare
Moved docs from the config file to the Configuring Guide. |
The generated config file defaults to
|
6d59427
to
92cf6cd
Compare
Your latest comment looks good 🎀 |
# so introduced as a config to ensure apps made with earlier versions of Rails aren't affected when upgrading. | ||
# Do not halt callback chains when a callback returns false. This is a new | ||
# Rails 5.0 default, so it is introduced as a config to ensure that apps made | ||
# on earlier versions of Rails are not affected when upgrading. |
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 seems like quite a story, particularly given that the only people who see this (new 5.0 apps) really shouldn't care about it at all.
Have we previously been this verbose on similar config values?
(This is more of a general observation about the comment as it already stood, unrelated to your specific changes.)
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.
There is the same addition in active_record_belongs_to_required_by_default.rb
.
This change could be removed from this PR and added to another one for both config files.
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.
The changes were in 569e03d
@claudiob I hesitate because technically, the default is currently just |
Ok then fine with closing it 😁 |
92cf6cd
to
bdf280b
Compare
Closing them. |
I just updated the PR. The configuration still needs to be explained in the docs. |
…onfig-halt-callback-chains Clarify config settings for AS::halt_callback_chains_on_return_false
Despite the config name, it isn't super clear when to use true or false for this config and which of the two is the new default. I remember searching in the docs to make sure I got it right. Clarifying it here might help others to be sure they got it right, too.
[skip ci]