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

Make positionals opt-in when strict:true #116

Merged
merged 4 commits into from
Apr 18, 2022
Merged

Conversation

bakkot
Copy link
Collaborator

@bakkot bakkot commented Apr 17, 2022

Fixes #85.

In the second commit I've updated the message for unknown options to suggest using --. I'm not sure if this is worth doing, but I figured I'd implement it for discussion; happy to revert if it seems unhelpful.

@shadowspawn
Copy link
Collaborator

From #85 (comment)

That is, I think that making positionals opt-in is in keeping with the "you will need to provide config for each option that you want to accept" design. I regard positionals as being a kind of option that you want to accept.

I think giving authors insight into the design helps smooth the experience, so more likely the API works the way they expect it to work because we guided their expectations (#85 (comment)).

I am interested in seeing a sentence or two in the docs to spin the config-what-you-accept story or similar. I was thinking outside the documentation of the properties themselves and tying in the options.

@bakkot
Copy link
Collaborator Author

bakkot commented Apr 17, 2022

I am interested in seeing a sentence or two in the docs to spin the config-what-you-accept story or similar. I was thinking outside the documentation of the properties themselves and tying in the options.

Makes sense. I've added the following sentence as a start, though I'm happy to try to expand this section:

Takes a specification for the expected options and returns a structured object corresponding to passed arguments.

@@ -141,11 +142,13 @@ function storeOption(longOption, optionValue, options, values) {
const parseArgs = (config = { __proto__: null }) => {
const args = objectGetOwn(config, 'args') ?? getMainArgs();
const strict = objectGetOwn(config, 'strict') ?? true;
const allowPositionals = objectGetOwn(config, 'allowPositionals') ?? !strict;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A brief comment on change in previous design/expectations.

This means parseArgs may now throw in strict:false mode for userArgs for first time. But it is opt-in and I think make sense when write it first time, and makes sense to read later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought this was better than making it so you could have an explicit allowPositionals: false but still get positionals. The alternative is to reject allowPositionals: false in combination with strict: false, but I didn't see much value in that.

README.md Outdated Show resolved Hide resolved
Co-authored-by: John Gee <[email protected]>
@shadowspawn
Copy link
Collaborator

Optional: there could be a single check for ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL after parsing, rather than two checks inside the loop.

Copy link
Collaborator

@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.

One optional suggestion. Nice tests.

@bakkot
Copy link
Collaborator Author

bakkot commented Apr 18, 2022

Optional: there could be a single check for ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL after parsing, rather than two checks inside the loop.

Doing the check after parsing means any other errors will happen first, even errors which are later in the input. I prefer getting errors in the order in which the corresponding problematic things appear.

@@ -86,12 +87,12 @@ function getMainArgs() {
* @param {boolean} strict - show errors, from parseArgs({ strict })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: missing JSDoc for new parameter.

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, thank you for the contribution 👍

@bcoe bcoe merged commit 3643338 into pkgjs:main Apr 18, 2022
@shadowspawn
Copy link
Collaborator

In the second commit I've updated the message for unknown options to suggest using --. I'm not sure if this is worth doing, but I figured I'd implement it for discussion; happy to revert if it seems unhelpful.

Something for future, not urgent.

We didn't really have time to discuss this. I expect most errors for unknown options will be for typos in the option name, so the suggestion is not going to be relevant. Not harmful, but not helpful. We are going the extra mile with error messages since they may be the UX, so could perhaps have two suggestions, with a "check the spelling" type suggestion first?

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.

Should positionals be opt-in when strict: true?
4 participants