-
-
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
#395 fix, short options do not understand adjacent values #599
Conversation
It looks like this pull request works with It would be nice if there was a standard we could refer to for short flag handling. Should we only consider numbers? Keeping pull request open for now. |
Link to issue: #395 |
Found a couple of posix references which align with suggested pull request, and python adds the last short flag taking a value.
% testopt -a -b
aflag = 1, bflag = 1, cvalue = (null)
% testopt -ab
aflag = 1, bflag = 1, cvalue = (null)
% testopt -c foo
aflag = 0, bflag = 0, cvalue = foo
% testopt -cfoo
aflag = 0, bflag = 0, cvalue = foo
|
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.
Are you still interested in doing more work on this @TheRoSS ?
I am now happy with supporting adjacent values. I have a few small changes I would like made, and want to do some testing. Checking whether you are still engaged before I go further.
No problem if you are no longer interested, I'll find somewhere to reference this work for future use by someone else.
Thank you for your contributions.
Good day. I am rather busy just now. But can participate in this issue in the end of May |
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.
Wish I had noted the changes when I first reviewed, but only one that stood out when I looked again was to follow the existing whitespace format (odd though it is!).
Merging into |
(I deleted some requested style changes that turned out to be stale, format on master had changed.) |
I made some minor changes, and already had one approval. Merged into v3 and will be included in that release. Closing to make it clear that should not be merged into master. Thank you for your contributions. |
Update: I am working my way through the parsing process looking into another bug. I suspect this fix only works for action based commands. |
Available now as a prerelease. See #1001 |
Shipped in v3: https://github.com/tj/commander.js/releases/tag/v3.0.0 |
In commander.js v3.0.0 (as a result of tj/commander.js#599) -wx is interpreted as -w with (invalid) timeout "x" instead of -w -x (which makes sense and is consistent with GNU getopt behavior). Split the options to avoid the problem. Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Example of the bug:
commander.option("-c ")
console.log("C:", commander.C)
$ node program -c234
C: -2