-
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
fix(halt-at-non-option): prevent known args from being parsed when "unknown-options-as-args" is enabled #438
Conversation
bump? |
5ac4d82
to
a0837d3
Compare
@bcoe anything I can do to get this merged? I would greatly appreciate having this fix generally available. |
@@ -3008,6 +3008,16 @@ describe('yargs-parser', function () { | |||
_: ['./file.js', '--foo', '--', 'barbar'] | |||
}) | |||
}) | |||
|
|||
it('is not influenced by unknown options when "unknown-options-as-args" is true', function () { |
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 would the parsed output be prior to your fix? This output seems good, I'm just not 100% understanding the original bug.
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.
I just realized I must have screwed up the test assertion when I cleaned up these commits a while ago. I just amended the assertion so it matches the expected unknown-options-as-args
behavior. Let me know if the output still looks good to you.
The full parsed output without the fix is
{ _: [ '-v', '--long', 'arg', './file.js', 'barbar' ], foo: true }
As you can see, yargs ignores halt-at-non-option
and parses the --foo
and --
options anyway.
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 for clarifying, this fix looks right to me 👍
…ons-as-args` is also enabled
a0837d3
to
4ff1b25
Compare
…nknown-options-as-args" is enabled
4ff1b25
to
7507bd8
Compare
@kherock thank you for the contribution. |
Closes #437
The test case included in this PR has a simple reproduction of a scenario I've encountered where I want parse all options up to a non-option. All options after this argument should be considered unknown, including
--
.Essentially, the use case that this accounts for is when a CLI provides two ways to indicate that the remainder of the argument list should not be parsed: by passing a non-option argument (enabling
halt-at-non-option
), or by passing--
(enablingpopulate--
). In addition to this, unknown options provided before the script are collected separately by enablingunknown-options-as-args
. In my use case, these unknown options are Node.js CLI args that I forward to a child process.