Repository navigation
Is strict actually a goal? #11
Description
Activity
It should be a goal to error and show help output when an unknown argument is passed, just like when a known argument is passed with an incorrect value.
"It should be a goal to error and show help output when an unknown argument is passed"
Agree.
"just like when a known argument is passed with an incorrect value."
Disagree. Also, a bit confused; I don't see any way the current spec allows you to define a value which validates against a value passed. The goal of this spec, which differentiated it from the previous work, was that you'd "bring your own validation" in reference to values & types.
@shadowspawn the F.A.Q. is fairly conversational in nature (as it literally spawned out of a discussion between myself &
@isaacs); with that in mind, it's correct in stating that we should parse (to the best of our abilities & with the options provided) all input.strictmode should throw an error when args are found that aren't explicitly defined/"known" inwithValues(or... maybe some other option which isn't spec'd yet?)(A different case is --foo=bar when foo is not specified to take an argument value. Not including that as part of strict for current question. nodejs/node#35015 (comment))
This makes sense... I'm actually a +1 to just kill
strictfor now. End-user can decide to throw if they see a flag they weren't expecting.@darcyclarke i meant conceptually - obviously this package wouldn't provide validation, but what's the use case for anyone wanting to permit unknown values?
@ljharb I know of usecases, today, where you'd want to capture unknown/undefined flags (to the root program) & pass those along to another process or use in some form later (ex.
npm's support of infinite config 👀 🤦🏻). Not saying it's a best practice, just that usecases exist.Contrived Example:
node mkdir.js ./path/to/new/dir/ --force --verbose --parents
// mkdir.js const knownOpts = ['force'] const { flags, positionals } = parseArgs({ withValue: knownOpts }) const args = Object.keys(flags).filter(f => knownOpts[f]) const cmd = (flags.force) ? 'sudo mkdir' : 'mkdir' process('child_process').spawnSync(cmd, [...args, ...positionals])
In the above, I expect & am aware of
--forcebut all other flags & positionals get passed along to a child process (note:mkdirwill then throw if it's not aware of those).Must we support every possible use case? Or perhaps it’d be better to establish good defaults, since nobody is forced to use this solution, and it’s perfectly fine if not everyone can.
Are we okay with dropping strict behavior for MVP? we can change this decision depending on code review on Node.js.
I'm still not sure why anyone would want non-strict - it seems much better to be strict by default, and let developers opt in to allowing unknown arguments.
Reference, two of the summary message from previous thread covering strictness and correctness:
Short version: I am ok with wrapping a release without
strictdetection of unknown arguments. But I am willing to invest in strict mode for the misuse errors in first instance, and then for unknowns.Long version
A big part of the original vision is the minimal config. This is incompatible with
strictsince flags are discovered at runtime and can't be "unknown", typos can not be detected by the parser.I think the minimal config is appealing and useful for quick programs used mainly by their own author. I am reassured that @bcoe thinks the non-strict mode is useful with his experience with Yargs and support issues.
To make
parseArgsuseful to a wider audience, I thinkstrictis important. This expands the audience to safer usage where the author can rely on common errors being detected and handled without writing their own code, and the end-user can rely on the program blocking many cases of accidental misuse. I want the utilities I write to become strict as they mature.I don't feel it is worth silently returning an error message when not
strict. (This idea is raised in a few places in the context of strict, and idea that parsing always succeeds.) I think that once an error has been detected then the parsing is tainted. Sostrictis a yes/no mode, the strict parse succeeded or it failed, and an exception is appropriate.I suggest as a proof of concept we implement
strictto throw for the error cases for a flag used with a value, andwithValueoption used without a value. These are within the scope of what is already described in the README and encountered in the current code and discussed at length in open issues. This is not what the README describesstrictas for, but I feel it is within the spirit!(As an aside, I continue to be impressed by how flexible Minimist is. Looks like is is capable of implementing strict for unknown arguments by using
opts.unknownand declaring the expected options.)I’ve used yargs in a half dozen projects, and always ended up adding strict later. Without strict, adding any argument is a breaking change. Strict mode is the only way semver-minor becomes practical.
I’ve used yargs in a half dozen projects, and always ended up adding strict later.
I suggest as a proof of concept we implement strict to throw for the error cases for a flag used with a value, and withValueI'm convinced we should have
strictfor MVP, but I'd make the case for it being an opt in, as with yargs.Would you be okay with this compromise @ljharb (needing to set a
strict: truein the options).26 remaining items
The caller can easily check, no loss of information, and no new pattern in result.
Well, they can, but will they? This is a persistent problem in JS: when the types are not as you expect, things end up giving the error at the wrong place.
How bad this is will depend on the results convention, but for simplicity I'm imagining something like what's proposed in that issue, where all the values for all options, boolean or string, end up in the same place. With such a design, if you write a script expecting
--path=/a/b, but the user just writes--pathpart, the script is going to get{ path: true }. If the script author forgets to check the type, and they do something likefetch(path), that's (hopefully!) going to fail with a message like "failed to fetchtrue". Which is not an especially useful error.
I think it's worth considering this with use cases in mind. For a rapid prototype or throwaway script, ending up with the wrong type for a configured option seems like it's just going to lead to more confusing errors, happening somewhat later in the execution. For authors hoping to provide custom errors, they now have to remember to check the
typeofof every returned option. Neither of those seem like they're well served by mixing options-used-correctly with options-used-incorrectly.What's the benefit of mixing the options-used-correctly with the options-used-incorrectly?
(Note that I'm only talking about the case where the script author does specify the option, but the user of the script misuses it, e.g. by passing a value to a boolean flag or failing to pass a value to a value-taking option.)
Well, they can, but will they? This is a persistent problem in JS: when the types are not as you expect, things end up giving the error at the wrong place.
This wider issue and comments from you and @ljharb is why I think we are currently leaning towards making
strict:truethe default, although the original vision was definitelystrict:falsewith BYO validation.What's the benefit of mixing the options-used-correctly with the options-used-incorrectly?
A range of responses, see if any resonate. I don't think your suggestion is bad, but I don't currently think it is better.
- The
strict:falsemode allows zero-config parsing. Using a variant of the minimist example, with proposed behaviours and results:
// console.log(parseArgs({ strict: false }); $ node example/parse.js -abc --beep=boop foo bar baz { passedOptions: { a: true, b: true, c: true, beep: true }, values: { a: true, b: true, c: true, beep: 'boop' } positionals: ['foo', 'bar', 'baz'] }
-
One way of looking at this is "the way they're stored matches the intention of the user, not the configurer, which will ensure the configurer can most accurately respond to the user's intentions."
Behaviour for zero config --foo=a ? #24 (comment) -
Counter question. If the author is not checking for errors, what's the benefit of quietly sticking some of the options somewhere else? This does not directly lead to better errors in the right place either.
- The
The
strict:falsemode allows zero-config parsing.You can have zero-config parsing either way. With the design I'm proposing, the only difference is that you'd read from the things-which-don't-match-config property of the result rather than the things-which-match-config property.
the way they're stored matches the intention of the user, not the configurer, which will ensure the configurer can most accurately respond to the user's intentions
I feel like the configurer can respond just fine either way? The only difference is whether we make it clear where the user's intentions don't match the configurer's.
If the author is not checking for errors, what's the benefit of quietly sticking some of the options somewhere else? This does not directly lead to better errors in the right place either.
If the author isn't checking for errors, then it doesn't necessarily lead to better errors, but it probably leads to earlier errors: you'll usually get a message about something being missing or undefined rather than having the wrong value entirely. And you're less likely to end up doing something completely wrong, like writing to a file named "true".
Conversely, if the author is checking for errors, it's easier to do so when all of the things-which-don't-match-config are in the same place, separated from the things-which-do-match-config.
I'm not all that attached to the idea of splitting out unknown properties with
strict: false, I should say. It seems like it helps, and does not hurt, both of the use cases we've identified, so I'm in favor of it, butstrict: falseis going to be require care to use anyway.But I do feel strongly that
strict: trueshould enforce (with an error) that options which are configured to take values are in fact given values.Thoughtful comments @bakkot thanks. I'm going to stop commenting on the idea of splitting out the un-authored properties to focus on the
strict:truehandling.(Explicitly adding my +1 to two of the cases I listed. 😄 )
- option of type='string' used like a boolean option e.g. lone --string
- I strongly support this throwing an error when
strict:true
- I strongly support this throwing an error when
- option of type='boolean' used like a string option e.g. --boolean=bogus
- I strongly support this throwing an error when
strict:true
- I strongly support this throwing an error when
The lack of detection of
--boolean=bogusis one of the things that I think broke the original proposal. See for example: nodejs/node#35015 (comment)@aaronccasanova wrote:
I also think this is the cleanest way to introduce strict mode by avoiding littering the parser with strict checks and allowing us to slip in the unknown option validation in the store option util
As for implementation, these two can both be detected in the same place as the unknown options in
storeOptionValue, and won't litter the parser. (No slight intended, I care about that too.)Reacted by Aaron Casanova- option of type='string' used like a boolean option e.g. lone --string
group of short options with an option taking a value in the middle
Short version: in strict mode, this could throw same missing value error as for lone
--string(identifying the short option which was expecting a value). I am +1 for that.Long version
I was writing up a long story when I realised this case does not need to be identified to the user as a different error. Here is the journey...
- I'll run through an example of the parsing that
getoptsand Commander do with this edge case. (I implementing the current Commander behaviour, so can speak to the reasoning if desired.)
If
parseArgsdoes the same and does not generate an error in strict mode:const result = parseArgs({ args: ['-foo'], strict: true, options: { f: { type: 'boolean' }, o: { type: 'sting' } } }); console.log(result.values);{ f: true, o: 'o' }- If the implementation blindly expanded all middle shorts in a group as if they were boolean, we would effectively be parsing
['-f', '-o', '-o']which would generate an error in strict mode due to the missing value for the first use of-o. I am ok with that outcome, but not with what the parsing would produce instrict:falsemode.
However, this does suggest the
strict:truemode does not need a different error, whatever thestrict:falsemode does. I like that... 💡Implementation: this will require extra code, but I already had this possibility in mind in #68 and been thinking further about it since.
Reacted by Kevin Gibbons- I'll run through an example of the parsing that
- added a commit that references this issue
on Mar 12, 2022 🔨 Moving towards MVP Milestone 1 (#87)
I propose:
- implement
strict:true. Throw errors for:
- unknown option encountered
- option of
type:'string'used like a boolean option e.g. lone--string - option of
type:'boolean'used like a string option e.g.--boolean=bogus
- strict by default (!)
- no error thrown or returned for
strict:false, left entirely to author
Why default to strict? Some of the active people here are very keen and eloquent! No champions here for
strict:false. Experience for (almost) zero-config is reasonable and visible opt-out:const { values } = parseArgs({ strict: false });Why not that error about short option group with string in middle? Rare. Get more important things done.
- implement
In strict, what about multiple values for something configured not to be multiple?
Not an error, last one wins.
Seems like in strict mode that should be an error, no?
No, definitely not. Arguments are always last-wins.
(For interest, this was raised and discussed in previous PR to node: nodejs/node#35015 (comment) )
Reacted by Aaron CasanovaReacted by Jordan HarbandStrict is indeed a goal, and enabled by default! Landed in #74.
The current interface is leaning towards the minimal (magic) configuration goal of the original proposal. I do not currently see a way of declaring an option as being known but not taking a value, so there is no way to identify unknown options?
The FAQ includes:
Related:
(A different case is
--foo=barwhenfoois not specified to take an argument value. Not including that as part of strict for current question. nodejs/node#35015 (comment))