-
Notifications
You must be signed in to change notification settings - Fork 10
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
Is strict
actually a goal?
#11
Comments
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. |
Agree.
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 & |
This makes sense... I'm actually a +1 to just kill |
@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. 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 |
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 Long version A big part of the original vision is the minimal config. This is incompatible with 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 I don't feel it is worth silently returning an error message when not I suggest as a proof of concept we implement (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 |
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'm convinced we should have Would you be okay with this compromise @ljharb (needing to set a |
@bcoe can you actually make the case? in what use case or realm of computing is non-strict a desired default? |
I think the main argument for making (The realisation that unknown arguments might be useful to detect comes later...) |
The easy thing should be the right thing; making the easy thing the wrong thing will cause problems later. Better to have a bit of friction in the beginning than a pile of tech debt later. |
I strongly agree with @ljharb. Not wanting validation (e.g. because you're doing it yourself in some way) is the unusual case. There's also another reason to prefer strict-by-default: if a user is intuitively expecting strict behavior, they may well never realize that it's not strict (CLIs are often not very well tested, especially for malformed input). So they could easily end up shipping something which isn't strict, which would bite the consumers of their script. (I recently took over a library which had exactly that problem with Conversely, if a user is expecting loose-by-default, it's presumably because they actually intend to pass input which would fail those checks. So they are much more likely to notice that their intuition is wrong, and realize they need to opt-in to skipping validation.
Even in casual cases, the config necessary to specify flags (or to opt out of |
The "Initial Proposal" just mentions |
@shadowspawn ☝️ this is what I don't like about
So, I'm not seeing how we would be strict by default without introducing quite a bit of ritual into how you configure your argument parser -- we need to add a config option that's purpose is "enumerate any keys that I care about that don't happen to have been defined in short, or withValue"... Edit: one decision we could make that would perhaps make strict less weird, would be if we always require that all short options have a long form, and are therefore defined in |
Couldn't it just be an additional That is, the thing I'd expect, as a user, would be to do something along the lines of util.parseArgs(argv, {
withValue: ['some', 'options'],
withoutValue: ['other', 'things'],
// maybe also `short` and `multiples`, which would be required to be subsets of the above two lists
}); |
Naming: Jordan Harband and I both thought of For strict I would suggest listing options in
For your interest and some context @bakkot, two of the four most downloaded argument parsers (npmcharts) default to zero-config parsing: yargs, and minimist. These two come from the same Optimist heritage, so not independent. The other two of the top four are strict by default and do not support zero-config parsing as such: Commander and argparse. These would seem less strange to you. 😄 |
Yup, I've looked at all of those before, and I do indeed find yargs and minimist to be extremely surprising. More to the point, I've regularly been bitten by applications built with those tools, because the author of the application failed to realize they needed to opt in to doing validation, which meant my typos were silently ignored. Which is a terrible experience all around. Number of downloads isn't a great metric because often it just means there's a single popular package which depends on the package in question. argparse, for example, is used by js-yaml, which radically inflates argparse's download count. Compare, for example, command-line-args, which has significantly fewer downloads than argparse but also has more dependent packages listed on npm (and more stars on GitHub, though again not a great metric). command-line-args is also strict-by-default, incidentally. |
That is a good insight from experience, thanks.
I wanted to draw the line on the long-tail of arguments parsers somewhere, and had wondered why the other metrics for argparse were so much lower. Thanks. I might stop talking about the Big Four and stick to the Big Three as my standard references for consulting, and then a wide variety of other interesting packages... |
Without member access to this repo I couldn't figure out an elegant way (with my fork) to branch off and PR against the proposed options API in PR #63, but I took a crack at extending it to support Here is the PR to Add |
Are people picturing that |
To be useful for BYO validation and changing the rules, I think I am imagining three modes of use:
|
I would expect strict true to throw errors, and strict false to build up an AggregateError of all |
It seems odd to create Errors and not throw them. If the user actually wanted those errors, they'd just use I was expecting that setting Another reasonable behavior would be to put the extra results on a separate object - so like In any case, it would be good to have a concrete use case or use cases in mind for Do we expect that setting |
Ignoring the errors is fine too, since someone who wants them can try/catch to get them (assuming we collect them all before throwing). I do also agree that unknown options, in sloppy mode, should be available on the results object. |
@bakkot wrote:
Correct. This is the original vision. Parse anything, zero-to-minimal config. If client wants to do something different with the auto-discovered options, they can identify them by comparing against the input configuration. |
@bakkot wrote:
Now we get to the nitty-gritty, which we have been skirting around until now and waving the strict-will-cover-this wand. 😄 Possible strict errors:
|
@ljharb wrote:
I don't understand this comment? |
@shadowspawn meaning, if we ignore the errors in sloppy mode, then someone who wants them can try/catch around a call in strict mode, and catch a thrown error to see what broke. Probably not the easiest, nicest, or fastest way to do it tho :-) |
Is anyone here a champion to drive errors in sloppy
Enough for MVP? |
@bakkot wrote:
@shadowspawn wrote:
Subjective opinion, but I feel
I also think this is the cleanest way to introduce |
I can give it a go after #63 is merged. I already have a WIP strict mode PR that builds off #63 that shouldn't be too difficult to add in some form of Also, let me know if anyone else is interested in championing. I'm simply stepping up in the interest of adding support for a basic strict mode in the initial release (Node v18 🤞 ) |
In strict mode, if I define an argument as a boolean and provide a value ( |
Yeah, I think that checking the values of options should be out of scope, but I feel like checking that The philosophy I'd like to advocate here is that you can create a well-behaved* script with *By "well-behaved" I mean that it will give the user helpful errors if they misuse it. Any script which does not do this is not well-behaved. That said: what should be the behavior for such misused flags for |
I have thought a lot about what to include in the returned options. I am proposing (duck-typing): If an option is used as a boolean, store result as for an option of The caller can easily check, no loss of information, and no new pattern in result. References
|
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 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 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.) |
This wider issue and comments from you and @ljharb is why I think we are currently leaning towards making
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.
// 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']
}
|
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.
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 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 But I do feel strongly that |
Thoughtful comments @bakkot thanks. I'm going to stop commenting on the idea of splitting out the un-authored properties to focus on the |
(Explicitly adding my +1 to two of the cases I listed. 😄 )
The lack of detection of @aaronccasanova wrote:
As for implementation, these two can both be detected in the same place as the unknown options in |
Short version: in strict mode, this could throw same missing value error as for lone 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...
If
However, this does suggest the Implementation: this will require extra code, but I already had this possibility in mind in #68 and been thinking further about it since. |
…ined short and value (#75) 1) Refactor parsing to use independent blocks of code, rather than nested cascading context. This makes it easier to reason about the behaviour. 2) Split out small pieces of logic to named routines to improve readability, and allow extra documentation and examples without cluttering the parsing. (Thanks to @aaronccasanova for inspiration.) 3) Existing tests untouched to make it clear that the tested functionality has not changed. 4) Be more explicit about short option group expansion, and ready to throw error in strict mode for string option in the middle of the argument. (See #11 and #74.) 5) Add support for short option combined with value (without intervening `=`). This is what Commander and Open Group Utility Conventions do, but is _not_ what Yargs does. I don't want to block PR on this and happy to comment it out for further discussion if needed. (I have found some interesting variations in the wild.) [Edit: see also #78] 6) Add support for multiple unit tests files. Expand tests from 33 to 113, but many for internal routines rather than testing exposed API. 7) Added `.editorconfig` file Co-authored-by: Jordan Harband <ljharb@gmail.com> Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>
🔨 Moving towards MVP Milestone 1 (#87) I propose:
Why default to strict? Some of the active people here are very keen and eloquent! No champions here for
Why not that error about short option group with string in middle? Rare. Get more important things done. |
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) ) |
Strict 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=bar
whenfoo
is not specified to take an argument value. Not including that as part of strict for current question. nodejs/node#35015 (comment))The text was updated successfully, but these errors were encountered: