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

Support options before arguments #193

Closed
agc93 opened this issue Aug 13, 2019 · 8 comments · Fixed by #1048
Closed

Support options before arguments #193

agc93 opened this issue Aug 13, 2019 · 8 comments · Fixed by #1048
Assignees
Labels

Comments

@agc93
Copy link

agc93 commented Aug 13, 2019

Especially for Unix/Linux platforms, the convention is usually to provide options before arguments:

Usage: grep [OPTION]... PATTERN [FILE]...
Usage: tar [OPTION...] [FILE]...
systemctl [OPTIONS...] {COMMAND} ...

Using boolean options before arguments currently gets incorrectly interpreted as assigning a value to the option flag. Would be good to support this ordering (preferably alongside existing behaviour).

@patriksvensson patriksvensson transferred this issue from spectresystems/spectre.cli Dec 30, 2020
@woutervanranst
Copy link

woutervanranst commented Nov 19, 2021

FYI I came here after looking through the issues. I believe the related error message is

       restore --keep-pointers c:\Users\Wouter\Documents\Test
               ^^^^^^^^^^^^^^^ Can't assign value

CommandParseException: Flags cannot be assigned a value.

This is not an issue with Options that have a value (eg. --keep-pointers true)
+1 on supporting the Linux/CLI convention

@dswisher
Copy link

dswisher commented Jun 8, 2022

Even as a developer, I was quite confused by the "Can't assign value" error message. Ideally, options would be allowed before arguments, but at the very least, could the error message be improved?

@FrankRay78
Copy link
Contributor

FrankRay78 commented Oct 18, 2022

Cake has an open issue closely related to this, cake-build/cake#3280, basically looking to move all the cake command options before the build script argument, allowing any options following the command argument to automatically become remaining arguments.

@FrankRay78
Copy link
Contributor

This issue is occuring due to the way that CommandTreeParser is handling Options, namely peeking ahead to the next argument to determine what to do.

The following image is when I've specified the following on the command line --version --somethingelse and it's currently parsing the --version Option. you can see that it's peeked ahead and discovered another Option:

image

It knows to correctly ignore the following somethingelse Option and just proceed with the 'No value?' code further on.

When I specify the following on the command line --version somethingelse, then when it is parsing the --version Option, it peeks ahead and discovers something following it of TokenKind String:

image

CommandTreeParser.ParseOptionValue proceeds to attempt to set the --version Option to this value, unsuccessfully:

image


For @woutervanranst 's example above to work correctly,

restore --keep-pointers c:\Users\Wouter\Documents\Test

I guess the CommandTreeParser needs to

  1. check if --keep-pointers is a flag (ie. boolean value specified on the command settings class)
  2. if yes, then ignore the peek, proceed with the 'No value?' code further on, and parse c:\Users\Wouter\Documents\Test on the next pass (which should be handled here I suspect:)
    image
  3. if no, then attempt to set the --keep-pointers option value to c:\Users\Wouter\Documents\Test

@woutervanranst
Copy link

I m assuming we don t need to reinvent the wheel here, I expect there are nasty edge cases we re missing - linux CLI parsers would have solved this already

@FrankRay78
Copy link
Contributor

I'm happy to have a go at this issue @patriksvensson - please assign it to me.

@patriksvensson
Copy link
Contributor

@FrankRay78 Done!

@FrankRay78
Copy link
Contributor

This issue can be closed/marked as complete @patriksvensson, now that PR #1048 has been successfully merged.

@FrankRay78 FrankRay78 linked a pull request Dec 11, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

5 participants