Cli: v2 feature: 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.

[lynncyrin]

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 (https://github.com/buildkite/agent/pull/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[bot]]

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[bot]]

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)

https://github.com/XANi/urfave-cli/commit/ba353ea41f636494934bd56ef01feb3770220ad5

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.