-
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
Improve API for configuring a high number of arguments #45
Comments
That seems like a useful semver-minor addition in the future, when usage patterns are well-understood. |
Fair question. I think the initial focus is on keeping it simple, and large scale CLI is not a concern for now. (And a large scale program could repackage the one-object configuration fairly easily.) |
Totally understand the desire to keep things simple. However, I don't believe the proposed suggestion adds complexity to the implementation or configuration. For example, I modified a few lines in the current implementation and enabled individual configuration objects: main...aaronccasanova:feat/improve-options-api const argv = ['--f', 'foo1', '--foo', 'foo2', '--bar', 'baz'];
const parsed = parseArgs(argv, {
args: {
foo: {
short: 'f',
withValue: true,
multiples: true,
},
},
});
console.log('parsed:', parsed);
Being that this is intended for Node core, I would have thought otherwise. Appreciate the quick response and feedback! |
My concern about including it now isn't about the code complexity, it's about the API surface, and the design space. |
For background, the Intended Audience from the "Initial Proposal". (May or may not reflect what eventuates!)
|
That's a helpful bit of context! Thanks for the reference @shadowspawn |
@aaronccasanova, I'm with @ljharb on erring on the side of a smaller API for MVP. With the goal initially being to support simple throw away CLIs and code samples. I'm hopeful we might be able to target Node 18, or Node 19 with the MVP, so the tighter we can keep the API the better, hence trying to keep behavior constrained. I think this work could immediate be a great follow up, and a good place to contribute to the Node.s codebase. |
@bcoe What about making this the only way to configure options? It seems more natural, to me: it means you're describing each argument in one place, rather than splitting up the configuration for each option across several fields. And if it's the only way to do it, it doesn't increase the API surface. const parsed = parseArgs(argv, {
args: {
foo: {
withValue: true,
},
},
}); does not seem noticeably harder to use than const parsed = parseArgs(argv, {
withValue: ['foo'],
}); and has the advantage of being a lot more natural as soon as you get at all beyond that: const parsed = parseArgs(argv, {
args: {
foo: {
withValue: true,
multiple: true,
},
bar: {
withValue: true,
},
},
}); is much easier to understand than const parsed = parseArgs(argv, {
withValue: ['foo', 'bar'],
multiple: ['foo'],
}); since the latter requires you to scan multiple lists to figure out how |
I think using nested objects is noticeably harder. The simplest configuration is a triply nested object. I think that is more challenging for a beginner to scan, or indeed type. Smaller comments:
|
I don't think "nested objects" as a concept is terribly confusing for most beginners. In my experience that's not the kind of complexity which usually trips people up when starting out. "These two things are implicitly related despite having no syntactic connection", on the other hand, definitely is. So I think the above approach is better even from a strictly pragmatic point of view when optimizing for the experience of beginners. That said, you can quite reasonably drop a level of indentation by having the second argument just be That is: const parsed = parseArgs(argv, {
foo: {
withValue: true,
multiple: true,
},
bar: {
withValue: true,
},
}, /* any other options */); |
@shadowspawn @darcyclarke @ljharb, if we went with the nested object approach for configuration, I feel like strict becomes more natural, since all your valid arguments are defined in one place ... For flags, we could allow any empty object for configuration, or flag: true? |
I’m ambivalent on either of the two ways to configure, but i feel strongly that non-strict is a very bad default, so if this argument form comes with default-strict, I’m super on board. |
I used a nested object structure for three reasons:
e.g. parseArgs(argv, {
strict: true,
...moreTopLevelConfigs,
args: {...}
})
e.g. type ParseArgsOptions = {
strict?: boolean
args?: {
[arg: string]: { // 👈 Isolated from top level configs
short?: string
withValue?: boolean
multiples?: boolean
}
}
}
We may not necessarily care about this here, but dropping a level of indentation will result in flaky types if we want to allow more top level configurations: e.g. How do you accurately type this example: parseArgs(argv, {
strict: true,
foo: {
short: 'f',
withValue: true,
},
bar: false, // Should error, not a valid arg value or top level config
})
// This would pass, but is no longer valid or representative of the implementation:
type ParseArgsOptions = {
strict?: boolean
[arg: string]: boolean | {
short?: string
withValue?: boolean
multiples?: boolean
}
} |
The suggestion was that options like |
Ah yes, looking more closely that is what you proposed 👍 I think my only nit with that API is when you want to set parser configs and no args configs: e.g. parseArgs(argv, null, { requireEqualSign: true }) vs. parseArgs(argv, {
requireEqualSign: true,
}) Otherwise, the drop level config you proposed looks great! |
This touches on something I had been wondering about, so I'll throw it into the mix! We have a default for the args input parameter which means it often won't be specified. If we do go strict by default, it might be better to make the input arguments the second parameter, or even a property on the configuration parameter, to avoid a clumsy invocation for the normal usage. e.g.
|
A nit.
(I raised the same naming issue previously in the context of the returned properties: #12) |
@shadowspawn I'm a fan of both suggestions:
parseArgs({
strict: true,
args: process.argv,
options: { foo: {...} },
}) |
I like the suggestion of: const parsed = parseArgs(argv, {
options: {
foo: {
withValue: true,
},
},
}); Personally, for whatever reason ergonomically I like passing first the arguments that you're parsing, then the description of how the parse shoudl take place. |
Me too! Although, while I have no data to back this up, I imagine the majority of users will leverage the default argv = getMainArgs(). As a result, we may see a lot of this out in the wild: |
I agree with both viewpoints - passing the args first is better, but being able to omit them is the many-nines use case. Short of providing two APIs - one that takes required args, and one that defaults them - i'm not sure how to reconcile those. |
Personally I think it makes more sense for the thing-to-be-parsed to come after the specification-of-how-to-parse. (Compare, for example, a tool like ajv, where you build a parser and then apply it.) I'm having trouble wrapping my head around the other viewpoint; can someone say more about why they prefer that? That said, I note that if you use an options bag, as in #63, there's no issues with ordering. |
"parse args" is a verb, "args" is the noun i want to parse, and "options" is how i want it parsed. |
I agree with that but don't understand why it implies anything about the arguments order. If anything it's the reverse: "use this specification to parse these args". |
Certainly one can form a sentence in either direction - "parse these args this way" versus "parse, this way, these args". |
@bakkot another argument I'd make for arguments and then options bag, is that it's what the community is accustomed to, as it's the approach taken by commander, yargs, minimist, args (worth noting I did find one exception meow). I'm pretty convinced that the combined popularlity of yargs, minimist, commander, and args, has enough momentum, that if we switch the order on people the community would constantly be screwing up the argument ordering. |
Are people constantly screwing up the argument ordering with I also note that the way I've usually seen commander and yargs used is as
command-line-args is much more popular than args and takes the options bag approach (its first parameter is an options bag with an optional |
(I should mention that, while it seems more natural to me to have the configuration parameter first, I'm not all that worried either way. I care more about the general shape of the configuration discussed in this issue than about the precise order of the arguments.) |
Short version: Commander only superficially follows the pattern, and I don't think is prior art supporting "arguments and then option bag" expectations. Long version For many years in Commander,
|
I'm fine with @aaronccasanova approach of providing I'm not seeing much argument in this thread against the spirit of #63, so I propose we land the work soon, since it seems to get us close to our goal of an MVP that we could put in Node 18. |
Per the discussion in #45 this PR restructures the current options API where each option is configured in three separate list and instead allows options to be configured in a single object. The goal being to make the API more intuitive for configuring options (e.g. short, withValue, and multiples) while creating a path forward for introducing more configuration options in the future (e.g. default). Co-authored-by: Benjamin E. Coe <bencoe@google.com> Co-authored-by: John Gee <john@ruru.gen.nz> Co-authored-by: Jordan Harband <ljharb@gmail.com>
Closing as #63 has landed. |
I wanted to play devils advocate on the design of the
parseArgs
options
object. Are there any concerns that theoptions
object will become unwieldy as the number of arguments grow? While I am still catching up on prior discussions and understanding the target audience of this tool, I imagine creating large scale CLIs (e.g.aws-cli
,git
,docker
, etc.) will be challenging to configure as configuring a single argument requires setting multiple top level keys (e.g.withValues
,multiples
,short
).I wanted to gauge folks opinion on allowing users to set all the configurations for a single argument in one object.
(Current) Example configuring large scale CLI options
(Proposed) Example configuring large scale CLI options
The text was updated successfully, but these errors were encountered: