Skip to content
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: parser should preserve inner quotes #407

Merged
merged 8 commits into from Nov 15, 2021
Merged

fix: parser should preserve inner quotes #407

merged 8 commits into from Nov 15, 2021

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Jul 20, 2021

Addresses: yargs/yargs#1324

Problem

Inner quotes are handled differently by parse, depending on the argsInput argument's type.

tokenizeArgString wraps argString with extra quotes if it is a string.
tokenizeArgString does not do the same for argString, if it is an array.
The first code block in processValue removes quotes from the value.

TLDR

If parse was given a string, extra quotes are added (tokenizeArgString) and then removed (processValue).
If parse was given an array, quotes are removed (processValue). This causes loss of inner quotes.

Example (using tests)

logs for string with inner quotes

{ argsInput: 'cmd --foo ""Hello"" --bar ""World"" --baz "":)""' }

during tokenizeArgString 
{ 
  argString: 'cmd --foo ""Hello"" --bar ""World"" --baz "":)""' }
  args: [  'cmd',  '--foo',  '""Hello""',  '--bar',  '""World""',  '--baz', '"":)""'  ]
}

after processValue 
{
  argv: [Object: null prototype] {
    _: [ 'cmd' ],
    foo: '"Hello"',
    bar: '"World"',
    baz: '":)"'
  }
}

result
{ args: { _: [ 'cmd' ], foo: '"Hello"', bar: '"World"', baz: '":)"' } }

logs for array with inner quotes

{ argsInput: [  'cmd',   '--foo',  '"Hello"', '--bar',  '"World"',   '--baz',   '":)"'  ] }

during tokenizeArgString 
{
  argString: [  'cmd',   '--foo',  '"Hello"', '--bar',  '"World"', '--baz',  '":)"'  ]
  args: [  'cmd',  '--foo',  '"Hello"', '--bar', '"World"', '--baz',  '":)"'  ]
}

after processValue
{
  argv: [Object: null prototype] {
    _: [ 'cmd' ],
    foo: 'Hello',
    bar: 'World',
    baz: ':)' // *** inner quotes missing ***
  }
}
result 
{ args: { _: [ 'cmd' ], foo: 'Hello', bar: 'World', baz: ':)' } }

Solution

I added conditional logic around removing quotes. I feel like my current solution is messy. I would love to hear your thoughts/advice on this. I know you're busy, so no worries if you don't have time. I will try to find a better solution.

There might be follow up work in yargs for handling positionals. The parsed object that gets returned by yargs-parser still has the positional arguments in argv._,

const parsed = this.#shim.Parser.detailed(...)

@jly36963 jly36963 marked this pull request as ready for review July 21, 2021 07:06
if (shouldStripQuotes) {
setArg(m[1], m[2])
} else {
setArg(m[1], stripQuotes(m[2]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since setArgs calls processValue, could this be simplified to:

setArg(m[1],, m[2], shouldStripQuotes);

Copy link
Contributor Author

@jly36963 jly36963 Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldStripQuotes should always be forwarded by setArg to processValue, except at this line. In this invocation, shouldStripQuotes should always be true.

I'm not a fan of the solution I have here

I try to keep code purely functional (whenever possible). In order to avoid reading global variables in setArg, I dynamically set the default value for shouldStripQuotes. I don't particularly like that pattern, so I'm down to change it.

@bcoe bcoe merged commit ae11f49 into yargs:main Nov 15, 2021
@bcoe
Copy link
Member

bcoe commented Nov 15, 2021

@jly36963 thanks for the patch 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants