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

feat(deps): introduce yargs-parser with support for unknown-options-as-args #1440

Merged
merged 1 commit into from Oct 7, 2019

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Oct 7, 2019

introduces unknown-options-as-args feature, which allows unknown options to be treated as arguments (either being collected inside of _, or passed as values to proceeding options).

CC: @henderea, @mleguen, @kellyselden

@bcoe bcoe merged commit 4d21520 into master Oct 7, 2019
@bcoe bcoe deleted the unknown-options-as-args branch October 7, 2019 00:49
@kellyselden
Copy link
Contributor

Excellent work!

@kellyselden
Copy link
Contributor

I'm still seeing the custom command behaviour here yargs/yargs-parser#181 (comment). Is that expected with the new option? Ref ember-cli/ember-cli-update#688.

@henderea
Copy link

henderea commented Oct 7, 2019

@kellyselden: my change wasn't related to the extra value in the _ array. I was fixing the issue with the positional parameters not accepting flags. I don't think you normally need to use the _ array when defining a command with yargs. It normally should work just fine to have a positional parameter, potentially a variadic one. This fix makes it possible to capture unknown options in a positional parameter, so I don't think there is any real need to use _ directly when defining a command. If you have positional parameters on the top-level command, you can define a command with the name $0 and that will allow you to parse positional arguments without relying on _.

As an example, if you have the command definition '$0 <exec> [...args]', and enable the unknown-options-as-args config option, you can parse the command node ./script.js ls -l . into {exec: 'ls', args: ['-l', '.']}. The _ might be something like ['$0'] or ['./script.js'] or whatever, but since you have the positional arguments parsed out, you don't need to use _.

@bcoe
Copy link
Member Author

bcoe commented Oct 7, 2019

@henderea @mleguen can you speak to @kellyselden's issue?

@kellyselden what would need to change to support ember's use case? perhaps you can summarize this in another issue, and we can try to make sure we support your use case.

@kellyselden
Copy link
Contributor

I can always do .slice(1) if it was intended. I was just trying to surface a potential issue in case the current command is not supposed to be in the list.

@bcoe
Copy link
Member Author

bcoe commented Oct 7, 2019

@kellyselden 👍, I'm currently buried in other issues, but will make an effort to take a look at your use-case soon (see if the behavior was intended or not).

@mleguen
Copy link
Member

mleguen commented Oct 8, 2019

@kellyselden _ indeed contains the executed command path and the remaining unexpected positionals.

I do not know if this was intentional - I have not been in this project for long -, but the code definitely looks like it was.

This is definitely undocumented :-)

However, it is useful when you read the parsing results outside of a command handler, as you can know the path of the executed command. The example for reset (although deprecated) in the doc uses this.

As you stated yourself, I would recommend using slice in command handlers when you need to get only the remaining positionals.

@bcoe
Copy link
Member Author

bcoe commented Oct 8, 2019

@mleguen @kellyselden I believe we originally didn't populate the command path, and people were concerned that they didn't have any way to determine context if they were using the same middleware for multiple commands.

@henderea
Copy link

Cool stuff:

$ node ./src/jse.js sudo ls -lh .
{
  _: [ 'sudo' ],
  args: [ '-lh', '.' ],
  '$0': 'src/jse.js',
  command: 'ls'
}

where the sudo command just does console.log(argv), and is defined as sudo <command> [...args]

So ya, works great on 14.2.0! Thanks for getting this merged in and released. I think the way _ is done is a good idea, because otherwise, the argv has no context about the command it is being given for. If you didn't have the command in _, you wouldn't have any ability to share handlers between commands. Not that that would be a common use case, but I think it's good to have the context somewhere, and _ is a spot that doesn't require taking up a possible argument or flag name. For example, my command is defined with the positional parameter <command>, so obviously there would be issues putting the yargs command name into the field command.

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

4 participants