Docs: Replace "off" with false [ci skip]
#49936
Conversation
| @@ -307,7 +307,7 @@ module ClassMethods | |||
| # [:allow_destroy] | |||
| # If true, destroys any members from the attributes hash with a | |||
| # <tt>_destroy</tt> key and a value that evaluates to +true+ | |||
| # (e.g. 1, '1', true, or 'true'). This option is off by default. | |||
| # (e.g. 1, '1', true, or 'true'). This option is +false+ by default. | |||
There was a problem hiding this comment.
Nitpick: I think "false" here should not use monospace because it is not referring to the false singleton. In this case, the default is actually (probably) nil, but that does not matter to the user, so we can say "false" to contrast with "If true".
There was a problem hiding this comment.
In this case, the default is actually (probably)
nil
@jonathanhefner I might be misreading the implementation, but the default options declare allow_destroy: false:
rails/activerecord/lib/active_record/nested_attributes.rb
Lines 351 to 355 in 8d778e0
I'm happy to remove the wrapping + characters, but I want to make sure that I understand the implications first.
There was a problem hiding this comment.
+false+ is inline with the description of :update_only: By default the +:update_only+ option is +false+.
Maybe also change If true, destroys to If +true+, destroys ? ...although I don't have a strong opinion here.
There was a problem hiding this comment.
Reading the link from @jonathanhefner I agree with his suggestion.
Do not document singletons unless absolutely necessary (as) that... allows refactors, and the code does not need to rely on the exact values returned by methods being called in the implementation.
There was a problem hiding this comment.
You are correct about it being false; I didn't actually look at the implementation and just assumed it was nil. But the point is that it doesn't matter to the user — indeed, the user will never see or interact with this value — and we could change the implementation to use nil without the user knowing.
In other words, the default is "falsy", just like the enabling value merely needs to be "truthy", not true. But we avoid those words in Rails documentation in favor of "false" and "true" without monospace formatting.
There was a problem hiding this comment.
@jonathanhefner thank you for sharing the link to https://guides.rubyonrails.org/v7.1/api_documentation_guidelines.html#booleans, I had no idea there were style guides for documentation! I'll read up on the rest of what's provided there.
In the meantime, with the Boolean documentation in mind, is this diff valuable at all? Is "false" an improvement over "off"? Should I replace +false+ with false, or would it be better to close this without change?
There was a problem hiding this comment.
We should not use +false+. I don't have a strong opinion on "false" vs "off", but if you think it reads more clearly, then I'm 👍.
There was a problem hiding this comment.
If we are going to change this, I think we should probably also change the following to use false and true?:
seanpdoyle
force-pushed
the
ar-nested-attributes-default
branch
2 times, most recently
from
3e839e4 to
3e42dc6
Compare
Replace a reference to a default `"off"` value in the `ActiveRecord::NestedAttributes` module with the actual default value: `false`.
seanpdoyle
force-pushed
the
ar-nested-attributes-default
branch
from
3e42dc6 to
1567419
Compare
|
Thank you, @seanpdoyle! 0️⃣ 1️⃣ 0️⃣ And thank you, @p8! 👋 |
Docs: Replace "off" with `false` [ci skip] Co-authored-by: Petrik <[email protected]> (cherry picked from commit 488a7ce)
Docs: Replace "off" with `false` [ci skip] Co-authored-by: Petrik <[email protected]> (cherry picked from commit 488a7ce)
|
I'm not sure this applies here, but reminded me of the guide for booleans. |
|
@zzak Jonathan mentioned that as well above: #49936 (comment) |
Detail
Replace a reference to a default
"off"value in theActiveRecord::NestedAttributesmodule with the actual default value:false.