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: wrap unknown args in quotes #2092

Merged
merged 6 commits into from Dec 28, 2021
Merged

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Dec 13, 2021

Addresses:
#2033

Problem

If an unknown argument is only whitespace, it causes confusing output: Unknown argument:

Reproduction

Code

// multiline.js

const yargs = require('yargs')

yargs(process.argv.slice(2))
  .command(
    '$0',
    'default description',
    yargs => yargs
      .option('opt1', { type: 'string' })
      .option('opt2', { type: 'string' }),
    argv => {
      console.log(argv)
    }
  )
  .strict()
  .parse()
# multiline.sh (version 1)

# space after backslash on opt1 line
node multiline.js \
--opt1=hello \   
--opt2=world
# multiline.sh (version 2)

# no space after last baskslash on opt1 line
node multiline.js \
--opt1=hello \   \
--opt2=world

Explanation

Calling multiline.js from multiline.sh (version 1) will result in an error like command not found. This makes sense, as having a space between the backslash and newline will prevent the newline from being escaped, and the newline would cause the next line to be interpreted as a new command.

Calling multiline.js from multiline.sh (version 2) will result in the unknown argument: error. I added a .map() call to the unknown arguments that will wrap them in quotes. This will make the error described in the issue a bit easier to understand: Unknown argument: " "

I don't know what might cause the situation in version 2. The issue specifically mentioned the error happening in a multiline run: | command in github actions. Maybe there's some process (like something in the github actions runner) that adds backslashes (where missing) at the end of each line within a multiline command. If something like this is happening, then extra backslashes would be added to the end of lines with trailing whitespace. I only have suspicions, and I feel like investigating it is outside the scope of this fix.

Solution

The solution mentioned in the comments section of the issue was to wrap arguments listed in the usage output in quotes, rather than trimming/mutating the elements of argv.

Questions

For the usage output:

  • Should I wrap all unknown arguments in quotes? Only whitespace ones?
  • Should I modify all similar usage output to also use quotes? (missing required argument: %s, etc)

@jly36963 jly36963 requested a review from bcoe Dec 13, 2021
@bcoe
Copy link
Member

bcoe commented Dec 16, 2021

@jly36963 If I'm understanding correclty, the problem is the form:

foo \[space]
--bar\
--final

And this would work?

foo \
--bar\
--final

I like your approach of making this jump out better to people.

@jly36963
Copy link
Contributor Author

jly36963 commented Dec 16, 2021

@bcoe

This causes the shell to throw command not found error, before the yargs program is run.

# newline is NOT escaped, --bar is interpreted as new command
foo \[space]
--bar\
--final

This causes yargs to throw unknown argument: error.

# newline is escaped, space is interpreted as an argument
foo \[space]\
--bar\
--final

(I think the second scenario would only happen if there is some process (before execution) adding backslashes at the end of lines that don't have them.)

bcoe
bcoe approved these changes Dec 17, 2021
Copy link
Member

@bcoe bcoe left a comment

I say ship it 👌this will at least help people identify this odd edge case.

@jly36963 jly36963 marked this pull request as ready for review Dec 17, 2021
@jly36963
Copy link
Contributor Author

jly36963 commented Dec 17, 2021

@bcoe
To be consistent with different kinds of error messages, should I modify all similar usage output to also use quotes? (missing required argument: %s, etc)

It wouldn't take very long

@bcoe
Copy link
Member

bcoe commented Dec 21, 2021

@jly36963 sounds like consistentcy would be worthwhile, did you want to do so in this PR, or merge this PR and have some follow up work?

@jly36963
Copy link
Contributor Author

jly36963 commented Dec 22, 2021

@bcoe
I can definitely do it in this PR, it should go pretty quick

@jly36963
Copy link
Contributor Author

jly36963 commented Dec 23, 2021

@bcoe
I'm happy with where it's at now. I took the minimalist approach: If an argument is just whitespace, it's wrapped in quotes; otherwise, it's left alone. I only did it for arguments, because I don't think there is a way to make ' ' an unknown command.

@bcoe bcoe merged commit 6a29778 into yargs:main Dec 28, 2021
7 checks passed
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