[cli] Improve type signature of CommandOption #11394
Conversation
erikareads
requested review from
TooTallNate,
EndangeredMassa,
trek,
onsclom and
jeffsee55
as code owners
🦋 Changeset detectedLatest commit: 02c2604 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if ( | ||
| option.name === 'env' || | ||
| option.name === 'build-env' || | ||
| option.name === 'meta' | ||
| ) { | ||
| argOptions[`--${option.name}`] = [String]; | ||
| } |
There was a problem hiding this comment.
comment (non-blocking): In future PRs, I would add a PR comment to code like this making it clear why this condition could be removed.
It took me a few minutes to figure out why this change was here and useful.
There was a problem hiding this comment.
I'll do that here by replying to your comment:
In this case, I can remove the extra condition since the CommandOption can now take constructors and [Constructor] directly as types, I don't need to rely on two sets of conditionals (the ternary above and this additional check) to get the parser type right.
Additionally, this was an interesting case, because these three commands specifically justified the multi: true in the CommandOption struct, but we can see here that we weren't using that information do the filtering and instead were hard-coding it.
The updated typing eliminates both issues.
Adds snapshot tests for the output from help as output by various commands. I expect to be changing some of the `Command` structs to continue the work started in [ [cli] Improve type signature of CommandOption #11394 ](#11394) especially as I build on work in [ Get flags specification #11433 ](#11433), and I want to be sure that the associated changes to help output are what we expect.
The previous type signature of
CommandOptiondid not cover all of the actual uses of option types throughout the codebase. This one may not cover all possible types, but comes much closer and allows us to literally state the type constructor used byargto parse options.This will allow for direct use of
command.tsdata structures so that Help output matches the actually parsed options used by every subcommand.deploy/index.tsalready shows an example of this, and a future pull request can build on that example by extracting that logic into a utility function.