-
Notifications
You must be signed in to change notification settings - Fork 120
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
String arguments that contain dashes do not parse properly #145
Comments
You can get around it with the bare -- option, but it was non-intuitive and took me quite a bit of digging in the debugger to understand what was happening. |
@andyburke good find, would you like to submit a failing test? |
Here's one: it('allows dashes in argument values', function () {
var argv = yargs("foo --arg '--bar --baz'")
.command('foo [options]', 'the foo command')
.describe('arg', 'Should contain some values')
.argv
// fails with AssertionError: expected true to equal '--bar --baz'
argv.should.have.property('arg').and.equal('--bar --baz')
}) |
Going to go ahead and bump this. I could really use this feature as well, and it's been awhile since this was updates. |
@bcoe Here's a failing test:
This fails (and apparently crashes the test runner) with:
I'm not familiar enough with yargs to figure out how to express this as a testcase that results in a single testcase failure instead of crashing mocha. |
The issue is actually arising in the separate yargs-parser library, so it should probably be addressed there. The argument string is tokenized correctly, but somewhere in the body of this loop (lines 147 to 305), the element corresponding to the contents of the quotes get broken apart and processed as options. |
Hi guys, any update on this one? I'm running into this issue for a CLI I'm writing https://github.com/tristanMatthias/wc-sass-render/. |
@tristanMatthias @masaeedu I've transferred this issue to yargs-parser, would happily accept a patch if anyone feels like take a stab at the problem 👍 |
@bcoe working on one now 😀 |
Ok @bcoe, I've submitted a pull request with the fix (#146). Please note the solution required a little workaround (detailed in the PR) in which I add an extra set of quotation marks (then remove them later in the parser). The reason for this is because previously the tokenizer would create an array with |
@tristanMatthias @andyburke having thought on this a bit more, here's the ultimate problem: node foo.js --hey "--test" '--test--'
[ '/Users/benjamincoe/.nvm/versions/node/v10.12.0/bin/node',
'/private/tmp/foo.js',
'--hey',
'--test',
'--test--' ] the operating system strips perhaps a better solution would be introducing an escape character, e.g., |
@bcoe I think the issue I'm having is a little different (maybe I need to open a different issue). When you pass something like the following: As far as escaping quotes goes, we can double up quotes so they survive the shell's expansion process and make it into node's Then it's |
@masaeedu aha! I'd missed this edge-case, interesting. @tristanMatthias thanks for a first pass at the solution, I wonder if we can figure out a way to avoid the temporary addition of quotes? working in the parser feels like a tech interview question doesn't it? |
It does indeed! Is there a way we can definitively differentiate between them with the quotes stripped? I thought about holding something in memory, but that doesn't work when it's passed from the Tokenizer to the Parser |
@bcoe btw, it isn't the operating system that's stripping quotes; it will faithfully forward whatever gets passed into This gets even hairier on Windows where things are precisely backwards; the system call for creating a new process expects a string instead of a list of arguments, so a shell like I'm just braindumping all this stuff i've learned about the surprisingly convoluted world of command line parameters that I've learnt over the past couple of weeks, this may already have been common knowledge for you and @tristanMatthias. |
The relevance of this stuff is that it's probably going to be pretty complicated to guess at what the original string and its quote wrapping was, because every shell can parse the string into an argv differently; whatever we get in the argv should be treated as the "source of truth". If a shell is stripping quotes before they ever make it into Node's argv, that should be avoided by using the shell's own rules for escaping and doubling up quotes. |
Sounds like this should be the final answer to me. I agree with @masaeedu. For us to escape these issues, we can also use the |
@andyburke @masaeedu @tristanMatthias please try:
I think it solves a bunch of issues around quotes and dashes in strings \o/ @tristanMatthias thanks for helping with the initial work, it got us going in the right direction. |
The Yargs parser doesn't seem to play nicely with option-like arguments that are passed as positional arguments. For more information, refer to the following issues: yargs/yargs-parser#381 yargs/yargs-parser#145 yargs/yargs#1821
Eg:
This will fall afoul of this case:
https://github.com/bcoe/yargs/blob/master/lib/parser.js#L103
I ran into this trying to encrypt a private ssh key using cryptex.
The text was updated successfully, but these errors were encountered: