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

reorder flags so they can come after arguments #1928

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions command.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ type Command struct {
// single-character bool arguments into one
// i.e. foobar -o -v -> foobar -ov
UseShortOptionHandling bool `json:"useShortOptionHandling"`
// Boolean to enable reordering flags before passing them to the parser
// such that `cli -f <val> <arg>` behaves the same as `cli <arg> -f <val>`
AllowFlagsAfterArguments bool `json:"AllowFlagsAfterArguments"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AllowFlagsAfterArguments bool `json:"AllowFlagsAfterArguments"`
AllowFlagsAfterArguments bool `json:"allowFlagsAfterArguments"`

// Enable suggestions for commands and flags
Suggest bool `json:"suggest"`
// Allows global flags set by libraries which use flag.XXXVar(...) directly
Expand Down Expand Up @@ -744,6 +747,11 @@ func (cmd *Command) parseFlags(args Args) (Args, error) {
return cmd.Args(), cmd.flagSet.Parse(append([]string{"--"}, args.Tail()...))
}

if cmd.AllowFlagsAfterArguments {
tracef("reordering flags so they appear before the arguments")
args = reorderArgs(cmd, args)
}

tracef("walking command lineage for persistent flags (cmd=%[1]q)", cmd.Name)

for pCmd := cmd.parent; pCmd != nil; pCmd = pCmd.parent {
Expand Down Expand Up @@ -1255,3 +1263,88 @@ func makeFlagNameVisitor(names *[]string) func(*flag.Flag) {
}
}
}

// reorderArgs moves all flags (via reorderedArgs) before the rest of
// the arguments (remainingArgs) as this is what flag expects.
func reorderArgs(cmd *Command, args Args) Args {
var remainingArgs, reorderedArgs []string

tail := args.Tail()

nextIndexMayContainValue := false
for i, arg := range tail {

subCmd := cmd.Command(arg)
if subCmd != nil {
cmd = subCmd
reorderedArgs = append(reorderedArgs, arg)
continue
}

// if we're expecting an option-value, check if this arg is a value, in
// which case it should be re-ordered next to its associated flag
if isFlag, _ := argIsFlag(cmd.Flags, arg); nextIndexMayContainValue && !isFlag {
nextIndexMayContainValue = false
reorderedArgs = append(reorderedArgs, arg)
} else if arg == "--" {
// don't reorder any args after the -- delimiter As described in the POSIX spec:
// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
// > Guideline 10:
// > The first -- argument that is not an option-argument should be accepted
// > as a delimiter indicating the end of options. Any following arguments
// > should be treated as operands, even if they begin with the '-' character.

// make sure the "--" delimiter itself is at the start
remainingArgs = append([]string{"--"}, remainingArgs...)
remainingArgs = append(remainingArgs, tail[i+1:]...)
break
// checks if this is an arg that should be re-ordered
} else if isFlag, isBooleanFlag := argIsFlag(cmd.Flags, arg); isFlag {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this call out to the top and use the return values if all the if/else instead of calling argIsFlag again

// we have determined that this is a flag that we should re-order
reorderedArgs = append(reorderedArgs, arg)

// if this arg does not contain a "=", then the next index may contain the value for this flag
nextIndexMayContainValue = !strings.Contains(arg, "=") && !isBooleanFlag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if for some reason the value matches the name of a command/subcommand then the logic breaks down right ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, if git has a subcommand remote but someone calls it with git -C remote then the correct interpretation should be that remote is the <path> value given to -C, it's a user error.


// simply append any remaining args
} else {
remainingArgs = append(remainingArgs, arg)
}
}

return &stringSliceArgs{append([]string{args.First()}, append(reorderedArgs, remainingArgs...)...)}
}

// argIsFlag checks if an arg is one of our command flags
func argIsFlag(commandFlags []Flag, arg string) (isFlag bool, isBooleanFlag bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func argIsFlag(commandFlags []Flag, arg string) (isFlag bool, isBooleanFlag bool) {
func *c *Command) argIsFlag(arg string) (isFlag bool, isBooleanFlag bool) {

if arg == "-" || arg == "--" {
// `-` is never a flag
// `--` is an option-value when following a flag, and a delimiter indicating the end of options in other cases.
return false, false
}
// flags always start with a -
if !strings.HasPrefix(arg, "-") {
return false, false
}
// this line turns `--flag` into `flag`
if strings.HasPrefix(arg, "--") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if strings.HasPrefix(arg, "--") {
arg, _ = strings.CutPrefix(arg, "--")

arg = strings.Replace(arg, "-", "", 2)
}
// this line turns `-flag` into `flag`
if strings.HasPrefix(arg, "-") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if strings.HasPrefix(arg, "-") {
arg, _ = strings.CutPrefix(arg, "-")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why, is this more efficient?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you dont need the if statement then. if string doesnt start with "-" it becomes a no-op

arg = strings.Replace(arg, "-", "", 1)
}
// this line turns `flag=value` into `flag`
arg = strings.Split(arg, "=")[0]
// look through all the flags, to see if the `arg` is one of our flags
for _, flag := range commandFlags {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to also think about short options handling here. What happens if the user gives multiple options like -fhsk where each of the f, h, s & k are bool flags

for _, key := range flag.Names() {
if key == arg {
_, isBooleanFlag = flag.(*BoolFlag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the internal boolFlag interface to check if flag is a bool flag.

https://github.com/urfave/cli/blob/main/flag_impl.go#L17C6-L17C14

return true, isBooleanFlag
}
}
}
// return false if this arg was not one of our flags
return false, false
}
Loading