-
-
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
added addArguments method #1467
Conversation
@shadowspawn This is a draft with the mvp for addArguments method. can you take a look and tell me what you think and what are the chances for a merge, before I continue to tests, docs and qa? |
Modified code less than I expected for first draft, which is nice for reading the diff. Sticking to the features which are already supported (argument name and argument description) keeps it simpler.
50% I am actually liking this more by leaving out the default value and custom coerce functions for now. There is room in the syntax to add them later if there is demand. |
I wouldn't release |
@shadowspawn do you think we should deprecate |
At least to some extent, yes. I am thinking the description hash would get dropped from the README, but maybe still mention briefly that can use |
What about the tests? Can I convert the tests that use arguments to tests that use argement and add argument, in order to verify that everything is working? It will be more convenient to change the tests instead of duplicate everything |
The old functionality is not being removed (yet), so the existing tests are still relevant to prevent regressions. |
@shadowspawn this pr is ready for review |
Oh drat, I have been making comments but they were not visible yet as I had not started a review. I wondered why you were ignoring them! |
FYI: opened an issue to poll reactions to API changes in #1471 |
One thing I have been wondering about but undecided is whether program
.command('build <target>')
.argument('target', 'source file for compilation') |
It's quite redundant because if someone will add the |
I think all comments are resolved |
I had a go at consistently using Argument class for I added a draft PR to your branch. It led me to wonder whether to be more lenient with what should be done with incorrectly formatted argument. e.g.
|
I think that the direction we should go to should be towards more explicit syntax, for example I would prefer using something like |
program | ||
.version('0.1.0') | ||
.addArgument(new Argument('<username>', 'user to login')) | ||
.argument('[password]', 'password') |
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 description from arguments.js
would be better, 'password for user, if required'
.
@@ -0,0 +1,7 @@ | |||
const commander = require('../'); | |||
|
|||
test('should throw on bad 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.
The test naming convention being used is:
- when something then result
I am comfortable with We are using two patterns, with either getter/setter depending on whether an argument is passed, or setter with no argument or optional argument. The pure setters sometimes have a verb like
|
Anyways, it was just a general thought, it's not really important for this PR, we'll discuss it further in the future if I want to add aa |
@shadowspawn The PR is ready on my side, if you need something else add more comments |
The two comments I had left are: (But they are small and I can fix them up later.) |
I am thinking of pulling this onto a branch and dong some more work on it, and including in Commander v8 in June. The changes to how non-conforming arguments are processed/rejected make it a breaking change for a small subset of users. (e.g. |
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.
Approving for merging onto a feature branch (for some further work).
Pull Request
Problem
inconsistent arguments and options syntax
Solution
introducing new addArgument function
ChangeLog
New command method:
addArgument()
and.argument()