Add missing + around a some literals.
#26905
Conversation
Mainly around `nil` [ci skip]
|
r? @chancancode (@rails-bot has picked a reviewer for you, use r? to override) |
| @@ -98,7 +98,7 @@ module Helpers | |||
| # to avoid warnings in the client about mixed media. | |||
| # Note that the request parameter might not be supplied, e.g. when the assets | |||
| # are precompiled via a Rake task. Make sure to use a Proc instead of a lambda, | |||
| # since a Proc allows missing parameters and sets them to nil. | |||
| # since a +Proc+ allows missing parameters and sets them to +nil+. | |||
| # | |||
There was a problem hiding this comment.
Why +Proc+ here but not on the line above?
There was a problem hiding this comment.
Because @bogdanvlviv missed it? Will fix
| # * <tt>require_layout</tt> - If set to true and layout is not found, | ||
| # an +ArgumentError+ exception is raised (defaults to false) | ||
| # * <tt>require_layout</tt> - If set to +true+ and layout is not found, | ||
| # an +ArgumentError+ exception is raised (defaults to +false+) |
There was a problem hiding this comment.
Just wrapping false and true in pluses isn't always what we want. That implies literally passing true or false when e usually mean truthy or falsy (and not putting those words in a fixed width font is our way of saying that).
cc @fxn
There was a problem hiding this comment.
Yup, found it described here: http://guides.rubyonrails.org/api_documentation_guidelines.html#booleans
There was a problem hiding this comment.
There are hundreds of existing entries which have +true+ where the same condition applies and it's unlikely that anyone will call this method and it not be explicitly true or false.
There was a problem hiding this comment.
Oh yeah, I noticed that we don't have a test for _default_layout raising an error when require_layout is set to true 馃槃
There was a problem hiding this comment.
When we configure stuff or document default values, unless the underlying code is doing a == false and the value has to be exactly that, we document just a flag. That is, the method does not require that users pass singletons, it just relies on standard boolean semantics, so we do not document singletons. We document just what the method needs.
For example, if the value comes from something dynamic, users can spare a !! to make sure the call satisfies the contract, they just pass the value with boolean semantics down.
There was a problem hiding this comment.
@fxn there are loads of places that use +true+ in the docs but will accept a truthy value. Also whether it's true or +true+ is a very subtle way of indicating that difference to users who are likely to be novices. Perhaps we need to come up with a clearer indication of the difference?
There was a problem hiding this comment.
Agree Andrew, in practice it's a convention that has not worked very well.
Also, in code examples there is even no typography so historically some methods have needed to change their API because of examples like
obj.predicate? # => trueI brought this topic to discussion a while back in core, I think it needs to be addressed somehow, but we didn't reach any agreement.
| @@ -255,7 +255,7 @@ def _conditional_layout? | |||
| # true:: raise an ArgumentError | |||
| # nil:: Force default layout behavior with inheritance | |||
| # | |||
| # Return value of Proc & Symbol arguments should be String, false, true or nil | |||
| # Return value of +Proc & Symbol+ arguments should be +String+, +false+, +true+ or +nil+ | |||
There was a problem hiding this comment.
I think +Proc & Symbol+ should be separate, and perhaps the ampersand should be spelled out ("and").
|
@kaspth @jonathanhefner thanks for the review, perhaps next time we can do it before I merge it 馃槃 |
|
@pixeltrix yeah, sorry about that! Completely missed this PR when it was first opened 馃槃 |
|
Thanks for the review! |
Hi! 馃槃 Today was inspired to improve our docs!
I added missing
+around a some literals, mainly aroundnil.[ci skip]