Allow option to have multiple comma separated values#624
Allow option to have multiple comma separated values#624WesCossick merged 7 commits intosparksuite:masterfrom
Conversation
|
@WesCossick please take a look at this. |
|
When an option has |
Co-authored-by: Wes Cossick <WesCossick@users.noreply.github.com>
| // Define what organized arguments look like | ||
| interface OrganizedArguments { | ||
| command: string; | ||
| data?: string | number | string[] | number[]; | ||
| flags: string[]; | ||
| options: string[]; | ||
| values: (string | number)[]; | ||
| passThrough?: string[]; | ||
| } |
There was a problem hiding this comment.
In this file, data (which also offers an acceptsMultiple option) can either be a single value or an array. But for values, we're keeping it as a comma-separated value until the src/utils/construct-input-object.ts file. That seems a bit inconsistent. Should the src/utils/get-organized-arguments.ts be storing the values as an array?
There was a problem hiding this comment.
Well, in some places, the array (eg: flags, options) are used as an indexed array, the index being determined by the position of the flag or option in the list of supported flags or options. In other cases, it's actually the same list that ends up in the object that's handed back to the application.
My aim with this change is not to re-engineer the waterfall-cli internals overall, just to add the ability for the existing design to handle passing back the list of comma separated options.
If you want to re-engineer how waterfall-cli does what it does, let's open a separate issue for that.
There was a problem hiding this comment.
I wouldn't consider it re-engineering, but rather, making sure this implementation fits within the current design pattern. The current implementation seems to be introducing a new pattern whereby src/utils/construct-input-object.ts needs to handle the acceptsMultiple option, even though that file doesn't need to currently for data.
There was a problem hiding this comment.
Here's the change to src/utils/get-organized-arguments.ts Claude suggested when asked:
Not including new tests, Claude seems to think that only a change to the src/utils/get-organized-arguments.ts file is necessary, not src/utils/construct-input-object.ts.
There was a problem hiding this comment.
Interesting. I've tested what you shared, and it seems to work too. I don't mind going with your patch if you'd prefer to do that, in terms of the parsing.
There was a problem hiding this comment.
Note: my testing is focussed on the multiple string values, not the numerics.
There was a problem hiding this comment.
If the patch gives you a head start that makes it easy to implement correctly & completely, versus what we discussed in person regarding saving some of the work for later, it'd make sense to do it correctly & completely from the outset.
There was a problem hiding this comment.
Latest changes works for multiple strings and number options.
There was a problem hiding this comment.
It seems like this integer and float validation won't work for acceptsMultiple options. Is that correct?
There was a problem hiding this comment.
I have not tested that, but, I suspect you are correct.
There was a problem hiding this comment.
Most recent changes support multiple options too
|
Regarding #624 (comment)
I've modified the help logic that produces the 'accepts:' list to make it something that's automatically clarified, by dynamically adding (if it accepts multiple arguments), by adding 'comma separated' before the list of options is provided. eg: --holds, -h List of ingredients to omit. (accepts comma separated: ham, chicken, beef, bacon, *) |
2 participants
Closes #623
Currently, when multiple valid options are provided as separate pairs of option + value, only the first option value is presented to the application.
eg:
only
value1will be present withininput.options.optionNamestring array.This change allows the following:
and for the application to see both values within
input.options.optionName(which is a string array)If one of the values are not valid, it will be called out as such.
I've also included a test variant that specifically validates that the multi-value behavior is functioning as expected.