-
Notifications
You must be signed in to change notification settings - Fork 71
Finishing the project commands TS port #1449
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
Conversation
|
I'm going to wait for this to be merged before I put this up for review. |
camden11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few minor TS issues but otherwise this is looking good!
joe-yeager
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a few nits
types/Yargs.ts
Outdated
| export type YargsCommandModule<T, U> = Omit<CommandModule<T, U>, 'builder'> & { | ||
| builder: (yargs: Argv) => Promise<Argv<U>>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since CommandModule is an interface we can extend it and it might simplify the TS logic here.
export interface CommandModuleWithRequiredBuilder<T, U> extends CommandModule<T, U> {
builder: (yargs: Argv) => Promise<Argv<U>>
}
df00fca
Description and Context
This wraps up the work to port all of the project command files to TS. I had to make some yargs typing changes to make things work well. I'm trying to strike the balance between getting value out of the yargs types, and making this so clunky to use that it's too much of a hassle to follow the patterns.
I had to make a few changes:
makeYargsBuilderutil was incorrectly matching on command buckets for help output. For example, runninghs project create --helpwas returning the help output forhs project --hlep. I fixed this by matching on the last command that the user ran (e.g "create" inhs project create --help)CommandModuleinterface is a little annoying to use because it makes the builder optional, and changes the type from a simple function to something else. I created some overrides that simplifies that type for our use casesmakeYargsBuildernowScreenshots
TODO
Who to Notify