Skip to content
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

doc: expand description of parseArg's default #54431

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bakkot
Copy link

@bakkot bakkot commented Aug 18, 2024

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Aug 18, 2024
@shadowspawn
Copy link
Member

From past experience, it is hard being clear about the config setup, the passed arguments, and the parsed result! I often say command-line but that does not cover custom arguments.

The suggested text does not accurately cover strict:false mode.

I'll have a try, instead try changing the initial description:

- The default option value when this argument is not passed.
+ The default option value when the option is not included in the arguments to parse.

doc/api/util.md Outdated Show resolved Hide resolved
Co-authored-by: John Gee <john@ruru.gen.nz>
@bakkot
Copy link
Author

bakkot commented Aug 18, 2024

I always forget about strict: false. Happy to take suggestions for more precision there.

doc/api/util.md Outdated
Comment on lines 1429 to 1431
`type` property. When `multiple` is `true`, it must be an array. For
`string` arguments, if the option is passed it must still be followed by
a value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`type` property. When `multiple` is `true`, it must be an array. For
`string` arguments, if the option is passed it must still be followed by
a value.
`type` property. When `multiple` is `true`, it must be an array.

In my personal opinion, saying "The default option value when this argument is not passed." implies that if it passed, it must have a value.

Secondly, "it must still be followed by a value." makes it sound like the "default" option doesn't work on string arguments...

Copy link
Author

@bakkot bakkot Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes it sound like the "default" option doesn't work on string arguments.

The default option doesn't do anything when the option being specified is passed (except in strict: false), so that's a correct inference (as long as you don't skip over the "if the option is passed" part). Unless I'm misunderstanding you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "it must still be followed by a value" is redudant because of "The default option value when this argument is not passed.", but that might just be me.

@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 18, 2024
@shadowspawn
Copy link
Member

Another possible wording (which I used in a recent comment where I was trying to clarify behaviour):

The default value is only used when the option does not appear in the arguments to be parsed. 

@bakkot
Copy link
Author

bakkot commented Aug 19, 2024

Tried a new wording based on @shadowspawn's suggestion, WDYT?

default {string | boolean | string[] | boolean[]} The default value to be used if (and only if) the option does not appear in the arguments to be parsed.

Hopefully that's sufficient to prevent people from coming to (incorrectly) believe that setting this would allow passing string-based options without arguments in strict: true.

@shadowspawn
Copy link
Member

Nice! Big +1 from me.

Copy link
Member

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small but solid incremental improvement, reducing chance for confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants