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

feat!: switch type:string option arguments to greedy, but with error for suspect cases in strict mode #88

Merged
merged 24 commits into from
Apr 19, 2022

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Apr 3, 2022

Should we make options expecting arguments greedy or choosy?
See #79 for survey of behaviours.

How about greedy, but with explanatory error for arguments which start with a dash when in strict mode?!
See #77 (comment)
Closes #77

Greedy is consistent with POSIX and Commander when strict:false and simple to describe. In strict mode we throw an error due to the possible mistake by end user omitting argument with an explanation of how to proceed if not a mistake.

These arguments will be eaten in strict:false and throw a helpful error in strict:true.

  • --foo --
  • --foo -a
  • --foo -2
  • --foo --bar

Errors:

$ node index.js --with --foo
/Users/john/Documents/Sandpits/parseargs/my-parseargs/index.js:60
    throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(errorMessage);
    ^

ERR_PARSE_ARGS_INVALID_OPTION_VALUE [Error]: Option '--with' argument is ambiguous.
Did you forget to specify the option argument for '--with'?
Or to specify an option argument starting with a dash use '--with=-XYZ'.
...stack...

$ node index.js -w --foo
/Users/john/Documents/Sandpits/parseargs/my-parseargs/index.js:60
    throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(errorMessage);
    ^

ERR_PARSE_ARGS_INVALID_OPTION_VALUE [Error]: Option '-w' argument is ambiguous.
Did you forget to specify the option argument for '-w'?
Or to specify an option argument starting with a dash use '--with=-XYZ' or '-w-XYZ'.
...stack...

Notes:

  • construction of error object follows format decided in feat: Add strict mode to parser #74
  • suggest short form only if user used short form
  • not using user value in example as it may require quoting! e.g. --foo "--bar alpha *test"

Copy link
Collaborator

@aaronccasanova aaronccasanova left a comment

Choose a reason for hiding this comment

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

I see this is still in draft but looks good so far 👍

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@shadowspawn shadowspawn marked this pull request as ready for review April 15, 2022 10:41
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Apr 15, 2022

I wanted to take a serious look at how we handle --foo --bar and why. Originally asked the question quite casually in #25 when thinking about string used as boolean, then went deep on choosy vs greedy in #79 and #85.

I think it is a coin-flip for greedy vs choosy, in trade-offs and implementations, and the feedback was split. Because we default to type:'boolean' in zero-config, we are not pushed towards choosy to disambiguate zero-config situations.

I think this PR is a good compromise of POSIX compatible behaviour when strict:false and informative error for possible usage errors when strict:true. Greedy with safety rails, it you use them.

(Full disclosure, we could still do the fancy error message if we stuck with choosy mode!)

@shadowspawn

This comment was marked as outdated.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Apr 17, 2022

Bumping this for consideration in case being ignored because I never put it forward for MVP.

I proposed this behaviour in (#77 (comment)) for feedback before pushing it for MVP, and then the node PR got opened and things got busy. :-)

Feedback was 3 x 👍 (including me).

@bakkot
Copy link
Collaborator

bakkot commented Apr 17, 2022

I think this is probably the best behavior, though any of the possibilities seem OK. +1 from me.

not using user value in example as it may require quoting! e.g. --foo "--bar alpha *test"

I think

const arg = /^[a-zA-Z0-9\-_]+$/.test(value) ? value : `'${value.replace(/[\\']/g, '\\$&')}'`;

will omit quotes in cases where they're obviously unnecessary and include them in the remaining cases.

@bcoe
Copy link
Collaborator

bcoe commented Apr 18, 2022

@shadowspawn do we want to land this for MVP?

@shadowspawn
Copy link
Collaborator Author

I would like to. It would be a breaking change in future as changes the parsing behaviour in strict:false for the greedy vs choosy cases.

But it isn't essential.

Copy link
Collaborator

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

One recommendation, let me know what you think?

if (strict && isOptionLikeValue(optionValue)) {
// Only show short example if user used short option.
const example = (shortOrLong.length === 2) ?
`'--${longOption}=-XYZ' or '${shortOrLong}-XYZ'` :
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather than -XYZ, any reason not to make this --${longOption}=${optionValue}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Short version: Quoting.

I actually had that but then worried about quoting it correctly in all cases, on all platforms, so it would work if copied and pasted. Went for a short safe placeholder!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I'll have a look for a future PR at using the value in the error message if it is plain text.)

Copy link
Collaborator

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

LGTM, I will merge this tomorrow before I rebase the upstream pull request in Node.js.

@shadowspawn
Copy link
Collaborator Author

Moved high-level greedy tests into index.js for sharing upstream.

@bcoe bcoe merged commit c2b5e72 into pkgjs:main Apr 19, 2022
@shadowspawn shadowspawn deleted the feature/greedy branch June 5, 2022 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "type: string" options greedy?
4 participants