-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Factory routine to create a new unattached argument #1497
Factory routine to create a new unattached argument #1497
Conversation
Thanks. We have a rich pattern for Command then Option to follow! Not much to customise yet, but I do want to follow the same patterns. |
(I saw from a deleted commit that we missed the esm export too.) |
(Sorry, I should have waited to see if the ESM tweaks were coming back! 😊 ) |
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
(This will land on |
It would be better if you also update |
(I'll add some tests too, but I can do the tests afterwards.) |
(Added TypeScript and tests in a PR to the PR.) |
…e-for-argument Add TypeScript and tests for createArgument
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.
Thanks!
*/ | ||
|
||
createArgument(name, description) { | ||
return new Argument(name, description); |
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.
What are the benefits of adding this factory when you can use new Argument()
directly?
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.
The main purpose is so you can override the method and return a custom subclass of Argument, which will be used to create the arguments added by Command methods .argument()
and .arguments()
and .command('sub <file>')
.
This factory pattern was first offered with .createCommand()
where it is more likely to be used, but offering the same pattern for Option and now Argument.
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.
But why will you want to override the createArgument method when you can just pass any custom Argument class to addArgument?
Also, in order to create custom argument, you need to somewhat know the internals of the Argument class (in both the factory solution and the direct solution), so it sounds like a bad practice and a doc bloater, for probably a really rare use case. Instead of adding this factory to keep the the api aligned, I would consider removing all other factories...
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.
.createCommand()
was the first one added in #1191 to make Command more subclass friendly. .createHelp()
was added in major help refactor in #1365.
.createOption()
was added in #1380 to follow the pattern. We had established a tidy pattern for Command
which also worked for Help
, so following through and offering same support for Option
.
in order to create custom argument, you need to somewhat know the internals of the Argument class
Yes, that is implicit in writing a subclass.
I did consider whether to directly support subclassing at all for Command
, but it a useful approach for extending functionality especially in custom cases which are not common use cases and would not be integrated into Commander itself.
No description provided.