breaking(flags): add collect option #2247
Conversation
flags/mod.ts
Outdated
| // if (hasTypes && get(o, key) !== undefined) { | ||
| // throw new Error( | ||
| // `Option "${ | ||
| // name.length > 1 ? "--" : "-" | ||
| // }${name}" can be used only once.`, | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
Or should this be uncommented in the future? If so, please add TODO comment
There was a problem hiding this comment.
It depends on how we want to handle duplicate options. You said you would prefer to throw an error for duplicate options. But currently we have tests to make sure that options can be overwritten.
If we want to keep the current behavior, we can remove those lines.
If we want to throw an error message for duplicate options, we can uncomment them.
If we want to make it configurable, I can add a todo to do that in a separate PR.
There was a problem hiding this comment.
Current behavior (overwriting) makes enough sense to me. Let's try this as first pass.
flags/mod.ts
Outdated
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const [key] of Object.entries(flags.bools)) { |
There was a problem hiding this comment.
nit: I think const key of Object.keys(flags.bools) makes more sense here.
Is it ok only to loop through flags.bools? What about the case when flags.strings are set and collectable and argv doesn't have key for it?
Maybe let's add test cases for those cases?
There was a problem hiding this comment.
ah right, i changed the default value for collectible boolean args from false to []. I would also need to set default values for collectible string args. is this fine for you or would you prefer not to set default values for collectible options?
There was a problem hiding this comment.
It looks fine to me to set default values to collectible options.
|
@bartlomieju This makes the parsing behavior more explicit than implicit, and upgrading path is obvious. So I think this is worth trying. What do you think? |
flags/mod.ts
Outdated
| @@ -52,6 +52,9 @@ export interface ParseOptions { | |||
| /** A string or array of strings argument names to always treat as strings. */ | |||
| string?: string | string[]; | |||
|
|
|||
| /** A string or array of strings argument names to always treat as arrays. */ | |||
| collect?: boolean | string | string[]; | |||
There was a problem hiding this comment.
I guess nobody would use collect: true as most cli tools tend to have --help or --version and they shouldn't be collectable. Do you think of any use case of collect: true?
There was a problem hiding this comment.
yeah maybe it's not necessary. i can remove it.
|
Thanks for updating! |
flags/test.ts
Outdated
| ]); | ||
|
|
||
| assertEquals(argv, { | ||
| foo: ["bar", "baz"], |
There was a problem hiding this comment.
@c4spar
Ah, I realized we still collect values for arg when there's no string, boolean, collect options are specified. What do you think about making this case "baz"?
There was a problem hiding this comment.
Because we check hasCollect || hasTypes before checking collectable in setKey, setting any option name changes the parse result (see the example below):
> parse(["--foo", "bar", "--foo", "baz"])
{ _: [], foo: [ "bar", "baz" ] }
> parse(["--foo", "bar", "--foo", "baz"], { string: "x" })
{ _: [], foo: "baz" }
I think this behavior is a little hard to explain to the users. What do you think?
There was a problem hiding this comment.
How about changing hasCollect || hasTypes ? !collectable : (...) part in setKey to just !collectable?
There was a problem hiding this comment.
Also fine for me. I just thought that we want to keep the old behavior when no options are set, so that the flags module can be still used without defining any options. But i can change it if you want.
Current behaviour is roughly described here: https://github.com/denoland/deno_std/blame/0d2ba0f223d993fd31a0d10193de769ffb4a8886/flags/README.md#L69-L83 (But there is still an open question about negatable options.)
There was a problem hiding this comment.
keep the old behavior when no options are set
Changing the parse behavior based on such condition feels too complex to me.
so that the flags module can be still used without defining any options.
I think flags module is still usable without any options even if we removed the old behavior. Do you mean such change is too breaking to existing users?
There was a problem hiding this comment.
I think flags module is still usable without any options even if we removed the old behavior. Do you mean such change is too breaking to existing users?
i mean also with collectible args. but i think i agree, it can be confusing. i will remove it.
close #2246