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

util: support 'option.required' in parseArgs #44565

Closed
wants to merge 6 commits into from
Closed

Conversation

himself65
Copy link
Member

Fixes: #44564

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 7, 2022
@himself65 himself65 changed the title util: support 'options.required' to parseArgs util: support 'option.required' in parseArgs Sep 7, 2022
@himself65 himself65 marked this pull request as ready for review September 7, 2022 22:46
@himself65 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2022
@nodejs-github-bot
Copy link
Collaborator

@@ -1053,6 +1056,7 @@ changes:
times. If `true`, all values will be collected in an array. If
`false`, values for the option are last-wins. **Default:** `false`.
* `short` {string} A single character alias for the option.
* `required` {boolean} Whether this option is required. **Default:** `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the default depend on strict?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure what it should be🤔
But in my real code, I just check if it is a truely value.

Copy link
Member

Choose a reason for hiding this comment

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

No, I think the behaviour is independent of strict. Options which take a value generate an error in strict, but there is a difference between an option with a required option-argument (i.e. type='string') and a required option as proposed here.

lib/internal/errors.js Outdated Show resolved Hide resolved
@@ -44,6 +44,7 @@ const {
ERR_PARSE_ARGS_INVALID_OPTION_VALUE,
ERR_PARSE_ARGS_UNKNOWN_OPTION,
ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL,
ERR_PARSE_ARGS_REQUIRED_OPTION
Copy link
Member

Choose a reason for hiding this comment

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

This does not describe why it is an error. I suggest sayERR_PARSE_ARGS_REQUIRED_OPTION_MISSING

@shadowspawn
Copy link
Member

The other input configuration options are all validated.

// Validate input configuration.

ArrayPrototypeForEach(
ObjectEntries(options),
({ 0: longOption, 1: optionConfig }) => {
if (optionConfig.required && result.values[longOption] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

The options came from the user, and is subject to possible prototype pollution. There is a helper routine objectGetOwn to do this more robustly:

function objectGetOwn(obj, prop) {

(The result was made internally with null prototype so can be used more conventionally with safety.)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, even more paranoid and robust is optionsGetOwn:

function optionsGetOwn(options, longOption, prop) {

@@ -1085,7 +1089,8 @@ const options = {
short: 'f'
},
bar: {
type: 'string'
type: 'string',
required: true
Copy link
Member

Choose a reason for hiding this comment

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

I suggest not including required in the example, as it is a less common configuration. The majority of programs will not use required, and there is some small potential for confusion between options expecting an argument, and required options.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

How would this be combined with options such as --help or --version?

I assume that required positional arguments are far more common than required options, which are usually optional. How would that be addressed?

Also, as @shadowspawn mentioned in #44564 (comment), some frameworks intentionally decided against supporting required options.

@himself65 himself65 closed this Oct 17, 2022
@aduh95 aduh95 deleted the 20220906-parseArgs branch October 17, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util: parseArgs should support required in options[key]
7 participants