Relax ordering requirements for flags/commands to improve upon cli ergonomics

[XANi]

Checklist

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

What problem does this solve?

CLI ergonomics when editing/adding flags to command are very bad.

Quick example, consider how many times you have to move cursor back in commandline to go from

$ cmd query users

to

$ cmd --out=csv query --filter "project=kittens" users --with-permission=Deploy

vs

$ cmd query users --with-permission=Deploy  --filter  "project=kittens", --out=csv 

It also "reads" worse:
"output CSV on query project kittens on users with permissions to deploy:
vs
" query users that can deploy in project kittens, output csv"

Without forced ordering you can add a flag at the end at any point so ad-hoc usage of commands is much more streamlined, it also allows writing CLI scripts with arguments grouped by what makes more sense, not by how program author structured it.

Now the long winded example, consider the following:

• App has global flags applicable to all subcommands like
  • change output type (for example switch between machine-parseable and user-frien
  • server address
  • config/credentials pass
  • environment
  • etc
• Application has subcommand with their own flags user might want to iterate

Let's say app has a subcommand to query systems state

cmd query

Which has --filter string option, and this command have subcommands for various parts of system

cmd query nodes
cmd query services
cmd query users

with various sub-subcommand specific switches.
Let's say user wants to only see stuff about a certain project. No worries, we have --filter flag for it

$ cmd query --filter project=kittens

User now wants to see who has access to that project. But we have options just for that

$ cmd query --filter project=kittens users 

So far so good. Let's see which of them can deploy the application

$ cmd query --filter project=kittens users --with-permission=Deploy

Okay, now let's compare it with another project...

$ cmd query --filter project=dogs users --with-permission=Deploy

Now we need to go back 4 words (4x C-b) and we can start changing it. But we have few more projects to check? How about just move filter at the end ?

$ cmd query  users --with-permission=Deploy --filter project=dogs
flag provided but not defined: -filter

Okay, we can't. Bit annoying but it is only few commands.

$ for project in kittens dogs otters ducks ; do cmd query --filter "project=$project" users --with-permission=Deploy ; done

Right, it does what we wanted. Now onto writing that report. There is a csv output format so it should be trivial.

$ for project in kittens dogs otters ducks ; do cmd query --filter "project=$project" users --with-permission=Deploy --out=csv; done
flag provided but not defined: -out

Oh, right, go C-b NINE times because that needs to be after a command but before subcommand.

$ for project in kittens dogs otters ducks ; do cmd --out=csv query --filter "project=$project" users --with-permission=Deploy ; done

Want to bump it to verbose or enable debug ? Go back nearly to the start of the line. Want to set environment or target server ? Same. Only real option to have convenient ability to change the parameters used in whole app is duplicating them as command-local variables.

The migration manual mentions something that "it is more POSIX-like" but POSIX conventions never had a notion of subcommands in the first place, the "stuff after options" was not a subcommand but also command's parameters (usually file to operate on), and even now most of the say coreutils tools allow for having options in any place just for convenience (GNU's ls -la /tmp and ls /tmt -la have same effect for example)

Solution description

Every --option in commandline should be considered for a flag, from closest "leaf" subcommand to the root

so

$ cmd --env=dev query --filter type=sqldb
$ cmd query --filter type=sqldb --env=dev

should be equivalent. Sometimes even having options before subcommand makes sense, like

$ cmd --filter="type=sqldb" query users
$ cmd --filter="type=sqldb" query nodes
$ cmd --filter="type=sqldb" query services

Describe alternatives you've considered

Copying every flag to local "works" but it isn't exactly elegant, and messes up with generated help.

[coilysiren]

I'm tentatively in favor of this, we would just have to think about any potential unexpected impacts.

Only real option to have convenient ability to change the parameters used in whole app is duplicating them as command-local variables

☝️ In practice this is how I've solved this problem, I really didn't consider it to be a huge issue. I agree that it's not elegant, though.

[rliebz]

For my use case, I actually prefer the behavior that flags must be placed specifically next to their declaration site.

To give an example of why this is important—I have an application called tusk that allows users to dynamically define tasks they want to run. Those tasks may have arbitrary flags that users may also define. The application as a whole has a verbose mode, in which more information is printed than normal. Individual tasks however may also have a verbose mode. So the following commands could all be valid, and semantically distinct:

# Run tusk in verbose mode, tests in normal mode
tusk --verbose test

# Run tests in verbose mode, tusk in normal mode
tusk test --verbose

# Run both tusk and tests in verbose mode
tusk --verbose test --verbose

I am not opposed to adding this as an option, but it is an option that I would personally keep toggled off in my applications.

[XANi]

@rliebz I have thought about that problem but to even distinguish between which levels a given flag is defined in v2 you have to go thru hoops. I'm not sure whether that's even intended behaviour but I had to do something like this:

	app := &cli.App{
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name: "verbose1",
				Aliases:[]string{"verbose"},
			},
		},
		Commands: []*cli.Command{
			{
				Name:    "l1",
				Flags: []cli.Flag{
					&cli.BoolFlag{
						Name: "verbose2",
						Aliases:[]string{"verbose"},

					},
				},
				Subcommands: []*cli.Command{
					{
						Name:  "l2",
						Flags: []cli.Flag{
							&cli.BoolFlag{
								Name: "verbose3",
								Aliases:[]string{"verbose"},
							},
						},
						Action: func(c *cli.Context) error {
							fmt.Printf("v  %+v\n",c.Bool("verbose"))
							fmt.Printf("v1 %+v\n",c.Bool("verbose1"))
							fmt.Printf("v2 %+v\n",c.Bool("verbose2"))
							fmt.Printf("v3 %+v\n",c.Bool("verbose3"))
							fmt.Println("new task template: ", c.Args().First())
							return nil
						},
					},
				},
			},
		},
	}

to even find a way to do it and considering using duplicate flag names is not even documented it looks like an implementation quirk rather than intended feature (or I am doing it wrong?). v1 had distinction between flag and global flag but honestly I made more errors in using them than actual useful functionality...

[rliebz]

I'm actually still on v1. I'm not sure what the current behavior is for this, or what the impetus for changing it was.

[XANi]

@rliebz in v2 it would change some edge cases with using cli.GlobalType vs just cli.Type

but v2 has no differentiation of that.

[whyrusleeping]

I would love if this ordering requirement were relaxed, its my primary complaint about this library.

[XANi]

I'd be interested in implementing it. Haven't looked at code yet but I'd probably also add a flag to opt in for "strict" ordering if it won't mess up code too much.

[sheerun]

In principle it should be as easy as replacing "flag" import with https://github.com/spf13/pflag but it seems not synchronized with upstreams and we have few errors:

./context.go:111:39: undefined: pflag.Getter
./flag_float64_slice.go:140:12: cannot use f.Value (type *Float64Slice) as type pflag.Value in argument to set.Var:
	*Float64Slice does not implement pflag.Value (missing Type method)
./flag_float64_slice.go:158:26: impossible type assertion:
	*Float64Slice does not implement pflag.Value (missing Type method)
./flag_generic.go:83:12: cannot use f.Value (type Generic) as type pflag.Value in argument to set.Var:
	Generic does not implement pflag.Value (missing Type method)
./flag_int64_slice.go:139:12: cannot use f.Value (type *Int64Slice) as type pflag.Value in argument to set.Var:
	*Int64Slice does not implement pflag.Value (missing Type method)
./flag_int64_slice.go:154:26: impossible type assertion:
	*Int64Slice does not implement pflag.Value (missing Type method)
./flag_int_slice.go:150:12: cannot use f.Value (type *IntSlice) as type pflag.Value in argument to set.Var:
	*IntSlice does not implement pflag.Value (missing Type method)
./flag_int_slice.go:168:26: impossible type assertion:
	*IntSlice does not implement pflag.Value (missing Type method)
./flag_string_slice.go:151:13: cannot use f.Destination (type *StringSlice) as type pflag.Value in argument to set.Var:
	*StringSlice does not implement pflag.Value (missing Type method)
./flag_string_slice.go:155:12: cannot use f.Value (type *StringSlice) as type pflag.Value in argument to set.Var:
	*StringSlice does not implement pflag.Value (missing Type method)
./flag_string_slice.go:155:12: too many errors

[yob]

We recently upgraded from v1 to v2, and discovered this broke some customer scripts that were using flags after arguments. For now, we've reverted to v1 to avoid the breaking change (buildkite/agent#1286).

We don't have a strong view on whether the v1 or v2 behaviour is preferable, but the change in behaviour is a blocker for us. It'd be great if there was a way to opt-in to the v1 behaviour with v2, purely for backwards compatibility.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[alertedsnake]

I'm in the same boat too, avoiding upgrading because of the breaking change in behavior.

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[XANi]

I've wrote a bit of code to "merge" flags on each step (and remove duplicates if same)

XANi@ba353ea

But I'm not sure how to make flag actually populate parameters correctly in current incremental parsing - with this modification only parameters present at last subcommand in chain are being processed and rest is ignored e.g cmd --format=csv sub1 sub2 "parses" but the variable is not set, while cmd sub1 sub2 --format works.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[alertedsnake]

I'm definitely still interested in the resolution of this issue, and still sticking with v1 for now because of the drastic change in behavior.

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[decentral1se]

I'm migrating from v2 -> v1 for this, thanks for opening this ticket. I'm moving all global flags to normal flags so I can get the same flexible support. Folks are complaining a lot about this in our cli. It's a major usability concern imho. #1113 (comment) seems like a good step forward for v2.

[jtagcat]

In principle it should be as easy as replacing "flag" import with spf13/pflag but it seems not synchronized with upstreams and we have few errors:

Mentioned errors solved:

• Filler Type() for flag interface(s).
• Using own fork, where PR for Getter() implementation in pflag is merged.

Problem: Parser loops multiple times and goes a level lower in (p)flag, that is normally expected.
(tests failing) It errors to parse arguments, since on the first loop ((global?) arguments parsing), the (local) argument definitions aren't populated.

jtagcat/urfave-cli#pflag

[meatballhat]

@jtagcat Thank you for working on this problem!

I admit that I'm more than a bit nervous about depending on spf13/pflag 😅 because it was forked from https://github.com/ogier/pflag for whatever reason, but diverged (?) enough that it's not being kept up to date, and is not being directly maintained with much regularity (last commit at time of this writing was over a year ago).

That being said, switching away from stdlib flag has long been discussed in this repo and I'd very much like to explore the possibilities. One such exploration is at https://git.meatballhat.com/x/argh, although I wouldn't consider it production-worthy by any stretch, and I'm not happy with the API 🤷🏼. Needs more work!

No matter what route we take with an alternate parser, I think that it will greatly reduce backward-compatibility pains by targeting the v3 series via the v3-dev-main branch. WDYT?

[jtagcat]

To be honest, I'm not a fan of pflag either.

The concern we have with making it v3, as mentioned above, is that the change would be ready, but not available (waiting indefinitely on other v3 features). From my perspective, v2 behavior is utterly unusable on the CLI. If I could, instead of large major version iterations, I'd ship many major versions with a feature or two each, but it's leaving users behind, if anyone cares.

A few weeks ago, I explored the idea of writing a fork from zero, adopting a few ideas. Among them, pre-run variable validation-sanitization, shared and inheritable/-d flags (improving documentation generation, ordering, and overall complexity). Supporting the base, and few core ideas sounds more attractive than accepting less-used features. Being conservative makes it easier to maintain. Forks exist for a reason.
I settled on not making yet another thing, and moved on to my other doings for now. Ideally, I planned to make my own GNU parser, and a varname linter. But that's again, a lot of sacrificed time with future maintainer responsibilities.

Thanks for mentioning argh, I'll look in to it sometime.

[meatballhat]

@jtagcat I'm happy to start publishing releases in the v3 series, fwiw, with the stipulation that versions would have a beta suffix or whatever is necessary to ensure clarity about it being more volatile than v2. Does that change anything for you?

[jtagcat]

@meatballhat: Yeah, that'd work. Though, that doesn't change the fact I don't have the time to contribute meaningfully. I'm mostly booked(, and this is work for me). Every now and then I can take 1-2 day spurt chunks/bites.

My priority would be to:

1. A library to parse gnu-style arguments in a flat manner (no local/global).
   ParseArg() (similar to chromedp.Run) takes in generic structs for short and long arguments. Using any/interface{} has its limits. Might have to look in to code generation for the various types.
2. Basic cli library functionality: structure, flags and args. No help pages, but capability to add.
3. Shared, inherited (and reusable) flags logic.
4. ctx.String("name") linter (+ mass renamer?). ¹
5. Expanding the core functionality to be usable as a lite version of the library.
6. ??? This is already forecasting.

Footnotes

1. I'm not a fan of ctx.String("name") as it is rather bulky, and does not convey that it comes from a flag. Another thing I'd like to enable (supported by linter and pre-action flag/arg validation) is direct usage of flags and args. (While I'm confusingly brainstorming here, named args would be nice as well. '3rd argument' is rather opaque, and changing it is even more confusing, but unsure what to do with it.) ↩

[jtagcat]

@meatballhat I looked at argh. I don't really have any comments.

I sat down for https://github.com/jtagcat/harg (going crazy noises). Looks surprisingly promising.

reinventing™ a golang argument parser.

a day-night or 2 days of work is expected to reach v1:

• finishing short arg parser
• small refactor to comply with FORMAT.md
• code generation or en masse copy-pasta for input definition wrappers, and for getting outputs (probably urfave-style)

[dearchap]

@jtagcat Love your effort. Please make sure to add unit tests. In fact I would write the test code first and then keep coding !!!!!

[decentral1se]

Moar #1928