-
Notifications
You must be signed in to change notification settings - Fork 118
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!: rework collect-unknown-options
into unknown-options-as-args
, providing more comprehensive functionality
#202
Conversation
…allow flags to be used as values of other flags
@henderea thank you for digging into this, I've been dragging my feet for too long on the next release of yargs, because I was hoping to loop back around to try to dig into this issue. I've been the victim of my of my libraries' own success these days, and have been spread a bit thin on code review I'm afraid 😦... thanks for your hard work. @mleguen, could I perhaps loop you in to help review this. |
@bcoe my pleasure, but I am afraid I will not be available for a review before wednesday (my weekly OSS day at work). |
Wow! Just finished reading the extensive discussion in yargs/yargs#1243, #181 and #202 :-)... |
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.
This PR sounds OK to me, apart from the small refactoring suggestion.
However, I still have one question about your former PR: why do you use Array.forEach loops instead of Array.some, which would be shorter, more readable and more efficient. If it is once again a NodeJS 6 compatibility issue, perhaps you should mark it as a compatibility hack to be removed later, as you did for Object.values?
index.js
Outdated
@@ -361,7 +361,7 @@ function parse (args, opts) { | |||
// and terminates when one is observed. | |||
var available = 0 | |||
for (ii = i + 1; ii < args.length; ii++) { | |||
if (!args[ii].match(/^-[^0-9]/)) available++ | |||
if (!args[ii].match(/^-[^0-9]/) || (configuration['unknown-options-as-args'] && isUnknownOption(args[ii]))) available++ |
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.
As you are using configuration['unknown-options-as-args'] && isUnknownOption(arg)
rather frequently, and for the sake of readability, I would advise refactoring it into isUnknownOptionAsArg(arg)
:
if (!args[ii].match(/^-[^0-9]/) || (configuration['unknown-options-as-args'] && isUnknownOption(args[ii]))) available++ | |
if (!args[ii].match(/^-[^0-9]/) || isUnknownOptionAsArg(args[ii])) available++ |
@mleguen On the topic of the forEach stuff, I was largely following the pattern of the existing code. The comment I put about Node 6 compatibility on the I'll make the requested refactoring to reduce the |
@henderea OK, I can see now the source of this complicated use of Array.foreach in place of Array.some is checkAllAliases. I will submit a PR to improve this. |
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.
OK for me, thanks @henderea.
@mleguen: I've committed the suggested refactoring of |
According to https://node.green/, it seems that support for |
collect-unknown-options
into unknown-options-as-args
, providing more comprehensive functionality
@bcoe: since I don't seem to be able to re-open #181, I'm creating a new pull request. It turns out that the issue I was having with the command parsing after the previous pull request was that it uses an array arg, rather than the
_
array. I've updated my feature fromcollect-unknown-options
tounknown-options-as-args
so that it can also cover this case.I've tested this in the script I plan to use it in, and it works as I would expect now.