Skip to content
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

feat: add generics to OptionValues #1537

Closed

Conversation

tyankatsu0105
Copy link
Contributor

Pull Request

Problem

close #1536

Solution

TypeScript typings

  • add generics to OptionValues
  • add default generics to storeOptionsAsProperties and opts method

ChangeLog

@shadowspawn
Copy link
Collaborator

This looks like it is functional, but I want to experiment a bit. I 'm not expert with generics and the current signatures don't give the cues I expect. The any looks a bit odd here:

opts<T = any>(): OptionValues<T>;

Example usage:

const options1 = program.opts(); // weakly typed
const options2 = program.opts<MyOptionValues>(); // strongly typed, added by this PR

program.action((options: commander.OptionValue) => {}); // convenience for weakly typed param

@shadowspawn
Copy link
Collaborator

How about this? The extends is a sanity check that the passed type is a hash/dictionary.

interface OptionValues {
  [key: string]: any;
}

opts<T extends OptionValues = OptionValues>(): T;

@tyankatsu0105
Copy link
Contributor Author

Wow. Looks better than my PR.
I take your advice and will change the code.

@tyankatsu0105
Copy link
Contributor Author

done
@shadowspawn thanks for your kind advice :)

@shadowspawn
Copy link
Collaborator

Oh, even tidier with not needing to specify default. 😄

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested tests for index.test-d.ts with the // opts tests.

interface MyCheeseOption {
  cheese: string;
}
const myCheeseOption = program.opts<MyCheeseOption>();
expectType<string>(myCheeseOption.cheese);
// @ts-expect-error Check that options strongly typed and does not allow arbitrary properties
expectType(myCheeseOption.foo);

@shadowspawn shadowspawn changed the base branch from develop to release/8.x May 30, 2021 01:46
@shadowspawn shadowspawn changed the base branch from release/8.x to develop May 30, 2021 01:46
@shadowspawn
Copy link
Collaborator

shadowspawn commented May 30, 2021

The next release will be based on release/8.x branch, and the typings have had a refactor causing merge conflicts. Might be easiest to make a new branch from release/8.x and redo PR (or copy-down and resolve conflicts if you prefer).

I don't mind sorting it out though, so optional for you to change the base branch.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shadowspawn
Copy link
Collaborator

Reminder to self: don't merge onto develop branch

@shadowspawn
Copy link
Collaborator

PR moved to #1539

@tyankatsu0105 tyankatsu0105 deleted the feat/add-generics-for-opts branch May 30, 2021 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants