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

Allow flags to come after arguments #1950

Closed
3 tasks done
fiatjaf opened this issue Jul 9, 2024 · 14 comments
Closed
3 tasks done

Allow flags to come after arguments #1950

fiatjaf opened this issue Jul 9, 2024 · 14 comments
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this

Comments

@fiatjaf
Copy link

fiatjaf commented Jul 9, 2024

Checklist

  • Are you running the latest v3 release? The list of releases is here.
  • Did you check the manual for your release? The v3 manual is here.
  • Did you perform a search about this feature? Here's the GitHub guide about searching.

What problem does this solve?

cli -s file.txt and cli file.txt -s should work the same way because having to strictly type the flags before the arguments is annoying. Most unix utils allow flags after arguments so people are used to that pattern and a thing that is obviously a flag shouldn't be interpreted as an argument just because it comes after another argument.

Solution description

cli -s file.txt and cli file.txt -s should work the same way.
cli -s file.txt and cli --foo=bar file.txt -s -v --whatever=something should work the same way.

Describe alternatives you've considered

I've tried switching to other CLI frameworks, but they are all much worse than this except in this aspect.

I've tried frontporting a feature from v1 that reordered arguments manually before parsing them: #1928 (currently using this with a custom fork and replace).

@fiatjaf fiatjaf added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Jul 9, 2024
@XANi
Copy link

XANi commented Jul 9, 2024

Additional context:

With multi-command binaries it's pretty common pattern to do the "config the command" first:

cli --server 1.2.3.4 --password-file ~/.cli/production.creds

and then when using it to just put subcommand after

cli --server 1.2.3.4 --password-file ~/.cli/production.creds ls --help

cli --server 1.2.3.4 --password-file ~/.cli/production.creds show

cli --server 1.2.3.4 --password-file ~/.cli/production.creds show --filter asd

or outright alias it

alias cli-prod cli --server 1.2.3.4 --password-file ~/.cli/production.creds

cli-prod ls --help

cli-prod show

cli-prod --filter asd

"POSIX compatbile" is not good. People are not POSIX compatible.

@BlackHole1
Copy link
Member

In v3(alpha), this is already supported. You can verify it using: go get github.com/urfave/cli/v3.

Related PR: #1568 #1835

@fiatjaf
Copy link
Author

fiatjaf commented Jul 9, 2024

It's not.

@decentral1se
Copy link

@dearchap
Copy link
Contributor

dearchap commented Jul 9, 2024

Additional context:

With multi-command binaries it's pretty common pattern to do the "config the command" first:

cli --server 1.2.3.4 --password-file ~/.cli/production.creds

and then when using it to just put subcommand after

cli --server 1.2.3.4 --password-file ~/.cli/production.creds ls --help

cli --server 1.2.3.4 --password-file ~/.cli/production.creds show

cli --server 1.2.3.4 --password-file ~/.cli/production.creds show --filter asd

or outright alias it

alias cli-prod cli --server 1.2.3.4 --password-file ~/.cli/production.creds

cli-prod ls --help

cli-prod show

cli-prod --filter asd

"POSIX compatbile" is not good. People are not POSIX compatible.

@XANi I dont see how your example relates to this issue.

@dearchap
Copy link
Contributor

dearchap commented Jul 9, 2024

@fiatjaf Please make sure that there is a field that can be set in Command to enable the behavior. Also make sure you have test cases for

cmd subcmd1 subcmd2 -f -s -X

where subcmd1 defines these flags and redorder is configured ONLY on that subcommand. Unless you are strictly expected this usage only for single level subcmd. I anticipate that people will ask why it does not work for 2-3 level sub commands. This has to be thought out more thoroughly.

@fiatjaf
Copy link
Author

fiatjaf commented Jul 9, 2024

This is my feature request, that PR is just one possible but humble attempt at implementing it. I'm sure people with more experience with this code can deal with this better, hence this feature request.

@dearchap
Copy link
Contributor

dearchap commented Jul 9, 2024

@fiatjaf you may submit the PR and we can merge it but eventually over time it is upto the maintainers to maintain this code and move it forward. So I want to make sure we have a firm foundation since this is not a simple feature.

@decentral1se
Copy link

decentral1se commented Jul 10, 2024

Thanks for weighing in @dearchap 🎉

I'm trying to think through an example which allows a ReorderFlags: true to be set on the root command which is then propagated to all sub-commands. If we take the following example:

# rough definition
cli [--global] sub1 [--foo] sub2 [--bar] sub3 [--baz] arg1

# user chaotically types while hacking
cli sub1 sub2 --foo sub3 --bar arg1 --baz --global

# transparently reordered
cli --global sub1 --foo sub2 --bar sub3 --baz arg1

Then the re-order logic is "just": parse flags and return them to the position where they are "registered", i.e. Flags: []cli.Flag{fooFlag}. There is no guessing what the user wants and we follow the logic of the *cli.Command.

We just need to figure out a precedence convention for flag names overlapping? Perhaps it is "local wins"?

Thoughts?

@dearchap
Copy link
Contributor

@decentral1se Thank you for the example. The example supposes that these are bool flags correct ? If you had this

cli sub1 sub2 --foo 10 sub3 --bar arg1 --baz --global

for example we wouldnt reorder since foo is an int flag ? It starts getting tricky. So say we support only bool flags and we have shortoptionshandling enabled we would need to parse out

cli [--global] sub1 [-f] sub2 [-b] sub3 [-z] arg1
cli sub1 sub2 sub3 arg1 --global -fbz

to 

cli --global sub1 -f sub2 -b sub3 -z arg1

I think we need to start working on #1273 which would provide a foundation for this behavior. One of pre-exec functions would allow users to add flags dynamically and another of those pre-exec functions would do reordering.

@decentral1se
Copy link

decentral1se commented Jul 13, 2024

@dearchap that's a good point about non-bool flags. Yeh, I guess it's just the same rule: follow what is registered on the flag (vs. the command this time) - does it accept an argument? then bring that with the sort? Will try manual test along with #1928 great work @fiatjaf

stivesso added a commit to VintageOps/structogqlgen that referenced this issue Sep 1, 2024
I hit the issue urfave/cli#1950 with
urfave/cli, which makes it hard to Allow flags to come after arguments.

Hence, shifting to make use of Cobra as it allows a little more
flexibility.

Signed-off-by: Steve ESSO <[email protected]>
stivesso added a commit to VintageOps/structogqlgen that referenced this issue Sep 1, 2024
I hit the issue urfave/cli#1950 with
urfave/cli, which makes it hard to Allow flags to come after arguments.

Hence, shifting to make use of Cobra as it allows a little more
flexibility.

Signed-off-by: Steve ESSO <[email protected]>
@dearchap
Copy link
Contributor

Fixed in #1987

@fiatjaf
Copy link
Author

fiatjaf commented Oct 20, 2024

Whoa! Thank you very much!

@decentral1se
Copy link

Holy shit, incredible work @dearchap 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this
Projects
None yet
Development

No branches or pull requests

5 participants