peterbourgon / ff

Flags-first package for configuration
Apache License 2.0
1.34k stars 59 forks source link

Flags are ignored after positional arguments (v4 alpha) #124

Open antoineco opened 9 months ago

antoineco commented 9 months ago

While experimenting with v4.0.0-alpha.4 for a toy project, I noticed that flags located after any positional argument were ignored during parsing.

The issue can be reproduced with the objectctl example on the latest tag: https://github.com/peterbourgon/ff/tree/v4.0.0-alpha.4/examples/objectctl

Examples:

objectctl create key val --overwrite  # --overwrite is ignored (Overwrite=false)
objectctl create --overwrite key val  # --overwrite is parsed (Overwrite=true)
objectctl create key val --foo  # --foo does not yield an error
objectctl create --foo key val  # --foo yields an error ("unknown flag")
soniaappasamy commented 5 months ago

Ran into the same today, would be handy to be able to specify the flags either way.

peterbourgon commented 5 months ago
objectctl foo --bar baz quux

Is baz a parameter to --bar, or a subcommand of foo?

antoineco commented 5 months ago

It depends how the bar flag was declared. If it's a Boolean flag no value should be expected, otherwise the next argument can be expected to be the flag's value without ambiguity. Unless I overlooked an edge case.

peterbourgon commented 5 months ago

It depends how the bar flag was declared. If it's a Boolean flag no value should be expected, otherwise the next argument can be expected to be the flag's value without ambiguity. Unless I overlooked an edge case.

What about the case when --bar is a boolean flag, and baz is true, or false, or anything that passes strconv.ParseBool?

Or, consider

objectctl subcommand foo --undefined-flag bar

Do we parse --undefined-flag as a flag and error (because it's not defined)? Or do we parse it as an argument to subcommand like foo and bar?

antoineco commented 5 months ago

What about the case when --bar is a boolean flag, and baz is true, or false, or anything that passes strconv.ParseBool?

Fair point, I hadn't noticed that Boolean flags could take a value.


Intuitively I'd expect anything that looks like a flag (-x or --word) to be evaluated as a flag, unless located after a double hyphen (--). It's just the way most Unix tools work and a muscle memory that is hard to undo.

Of course if the library deliberately forces flags to come before any positional argument this is also fine, as long as this design decision is clearly communicated to consumers :)

antoineco commented 5 months ago

By the way, just to provide more context about what I was originally trying to do:

I have a command with a global flag --verbose/-v which enables verbose logging. Sometimes, the invocation of a command will fail and return the final error with some context, but not all. When that happens, I typically re-call the same command from my shell history, and append -v before pressing Enter to replay the actions with more details, such as the API calls that are being performed, etc. With a command implemented with ff, I need instead to travel before the subcommand's positional arguments to add -v.

peterbourgon commented 5 months ago

~Are you using ff v1/v2/v3, or ff v4?~

edit: I see in your OP that you're working with v4, mea culpa

charbonnierg commented 5 months ago

Hi, I'm using ff v4, and think I hit the same issue.

Below is the code which can be used to reproduce the problem (click to expand): ```go package main import ( "context" "errors" "fmt" "os" "github.com/peterbourgon/ff/v4" "github.com/peterbourgon/ff/v4/ffhelp" ) func cli() int { root := ff.NewFlagSet("example") debug := root.Bool('D', "debug", "enable debug mode") exampleCmd := &ff.Command{ Name: "example", Usage: "example [FLAGS] SUBCOMMAND ...", Flags: root, } // example server serverFlags := ff.NewFlagSet("server").SetParent(root) serverCmd := &ff.Command{ Name: "server", Usage: "example server", ShortHelp: "run example server in foreground", Flags: serverFlags, Exec: func(ctx context.Context, args []string) error { directory := "" if len(args) < 1 { return fmt.Errorf("missing directory") } directory = args[0] if *debug { fmt.Printf("starting server in directory: %s\n", directory) } return nil }, } exampleCmd.Subcommands = append(exampleCmd.Subcommands, serverCmd) err := exampleCmd.ParseAndRun(context.Background(), os.Args[1:]) switch { case errors.Is(err, ff.ErrHelp): ffhelp.Command(exampleCmd).WriteTo(os.Stderr) return 0 case err != nil: fmt.Fprintf(os.Stderr, "error: %v\n", err) return 1 default: return 0 } } func main() { os.Exit(cli()) } ```

I could not understand reading this thread whether this was the intended behavior or not. Do you believe that it should be possible to do that with ff and may work on it in the future or do you believe that ff cannot make choices which will be robust in all cases ?

Anyway thanks this awesome library

mfridman commented 3 months ago

I decided to kick the tires on v4.0.0-alpha.4, and some of the quality of life improvements we've gone back and forth on have been largely addressed, so thank you for taking the time to rethink parts of v3.

I respect your position, but it'd be great if you could reconsider this particular one.

In practice, the most common thing is being able to mix and match flags and args. If it quaks like a flag, it's registered as a flag, then it's probably a flag (and that's good enough for 99% of cases!).

There will always be some edge cases, and maybe for those situations there could be an escape hatch to "parse as raw" deferring all the logic to the CLI author.

peterbourgon commented 3 months ago

Just to peel back the curtain a bit: I definitely want to make this work. I've been stuck on how to do that without completely re-implementing how flags, flag-sets, args, etc. are parsed.