-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
doc: "if and only if" should be "if" in util.parseArgs default value #58958
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
upstream comment:
#54431 (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.
Re: #54431, the critical part is "the option does not appear in the arguments to be parsed".
However, the way it's currently worded "if and only if", it literally means that the value passed as
defaultbehaves as a sentinel value: when it matches, the option was missing; otherwise, the option was in the arguments provided.But that's not how it works. It's a one-side implication.
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.
Well it says "to be used", so I don't think it says the value could not be the default value if the option appears in the arguments.
Maybe we can try to rephrase it further to remove the ambiguity:
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.
@aduh95 That leaves out what
defaultis actually used for. Adding your suggestion to the previous text:Uh oh!
There was an error while loading. Please reload this page.
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 wording is also an "if and only if", just more verbose. This wording implies that if string option
--foohas a default value of"bar", then--foo=baris ignored.I'd go with this:
Uh oh!
There was an error while loading. Please reload this page.
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 added a new commit, with an expanded text that is hopefully clearer in the context of #54396 and #54431. Does this work for you, @shadowspawn ?
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.
What is this new text about:
even if the provided value is falsy? I don't understand why this has been added.Otherwise, I agree hopefully clearer, spelling out the
(if and only if). :-)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 is my takeaway from #54396. "falsy" meaning
x == false, but actually left open to interpretation, so that whatever false/empty/zero/value someone may think will lead to the parser entering defaulting mechanics, it will not be taken.Well, this was an
if (and only if)-note the distinct position of the parens. Whenever such half-parenthesized construct is used, it means that if some convenient assumptions are made, a backwards implication holds, but there are readings where the implication is only forwards. i.e. Considering those additional assumptions, the text turns out to mean notp <=> q, butp => q ∧ r <= s, where eitherpis equivalent tor, orqis equivalent tos.Sorry for beating the dead horse, but I think I didn't manage to properly justify how the previous text was problematic, and I just took the chance.
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.
That wasn't the issue, and thanks for asking me to take a look. The previous wording change was a bit difficult (like this one!) and clearly hasn't worked for everyone.
The original cause for a wording change was that someone came in hoping to allow:
The previous wording did not make it clear enough to them that they couldn't use a default value to cover that middle case.
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 wanted to liberally fit
util --fooin the "foo is falsey" when parsed as a string; but I forgot it would actually betrueif parsed as a boolean. I also wanted to make the "it is ignored..." sentence explicitly non-normative. But there's no such concept in Node.js docs. Documentation is hard. 😄