reorder flags so they can come after arguments #1928

fiatjaf · 2024-06-26T02:19:15Z

What type of PR is this?

feature

What this PR does / why we need it:

This brings back (and improves) the reorderFlags function from v1 that allows flags to be specified after arguments and still work. It's a very much requested feature, see #1733 and #976.

Which issue(s) this PR fixes:

Fixes #976

Testing

I've done some manual testing.

skelouse · 2024-06-26T02:41:01Z

I did something similar in another repo if you need this: #1583 (comment)

linrl3 · 2024-07-04T06:01:13Z

Some Run tests are failed, please check @fiatjaf

fiatjaf · 2024-07-04T14:03:53Z

I don't understand what is failing. Looks like it's test coverage only? I can fix that, but can you tell me first if this change is wanted and would be merged if tests were ok?

dearchap · 2024-07-04T16:54:15Z

@fiatjaf No one has specifically asked for this feature, so it depends. Also we have persistent flag support, so flags can be defined so that they can be placed anywhere in the chain

fiatjaf · 2024-07-04T18:08:54Z

I've linked to two issues above with many people asking for it. I've seen you replied on these issues with that comment about persistent flags, but I don't think it's related.

This is about specifying flags after arguments, not after subcommands. Like cli -s file.txt and cli file.txt -s should work the same way.

joshfrench · 2024-07-04T19:17:41Z

This trips up my team all the time — I’d definitely appreciate the feature!

dearchap · 2024-07-04T19:32:33Z

@fiatjaf This starts getting complicated. Can you provide more examples of the behaviour ? if you have say

cli subcmd1 subcmd2 -f -s -t

that would be equivalent to

cli -f -s -t subcmd1 subcmd2

even if say '-s -t' are defined for subcmd1 and not root command.

fiatjaf · 2024-07-04T20:09:01Z

Subcommands should not be involved here. So cli subcmd -s -t -m file.txt otherfile.txt should be equivalent to cli subcmd file.txt otherfile.txt -s -t -m.

dearchap · 2024-07-04T22:18:31Z

I understand the appeal of this but you are interpreting what the user want to achieve. Its is entirely possible that the subcmd wants to handle all the args itself in the command action.

cli subcmd file.txt s.txt -f -s

So you need a new boolean field in Command, something like ReorderArgs, which is off by default and can be enabled if a particular user of this library wishes it. Also you have to ensure that the reorder can be applied only to a leaf command and not to root/branch commands which have subcommands.

decentral1se · 2024-07-08T15:37:17Z

No one has specifically asked for this feature

checks notes one of the most critically requested features and reason for several projects migrating back to v1 and/or dropping urfave/cli altogether in favour of cobra 😆

abitrolly · 2024-07-09T08:33:59Z

checks notes one of the most critically requested features and reason for several projects migrating back to v1 and/or dropping urfave/cli altogether in favour of cobra 😆

[reference needed]. I don't see "reorder" in the critical list of requested features.

https://github.com/urfave/cli/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc

Alzo the v1 migration dataset is very interesting to see.

decentral1se · 2024-07-09T09:58:06Z

@abitrolly you don't have to look far #1113 Also see what @fiatjaf referenced? There are several folks (including myself) who had to remigrate to v1. Additionally, several folks there and in other issues (use search) migrated to cobra because of this. I don't know who curates this list but this ordering issue should be on it imho

Also, #1331 (comment) "its a long standing feature ask"

fiatjaf · 2024-07-09T10:06:45Z

I've opened a feature request here: #1950

Let's go there an upvote it!

XANi · 2024-07-09T10:10:49Z

Can confirm, can't move to v2 in any of our project for ergonomic issues. "It's more POSIX compliant" is a very weak arguments, humans are not POSIX compliant.

We have a lot of pattern that can be described as

<cmd> --global --config <subcmd> --subcmd-config

because for CLI user it is easier to just copy front part (<cmd> --global --config ) or outright alias it for shorter cmdnline and then change subcommand and its options when using it.

Forcing flags to be at certain position just means unnecesary cursor movement

decentral1se · 2024-07-09T15:20:39Z

I've done manual some testing @fiatjaf and it doesn't seem to be working for flags on sub-commands?

./<cli> <command> --help # (1)
./<cli> <command> <sub-command> --help # (2)

Where both (1)/(2) show help output for <command> only.

I can't seem to trigger help output for <sub-command> at all.

fiatjaf · 2024-07-09T15:48:11Z

That's true. It's broken indeed. I'll try to fix it soon.

dearchap · 2024-07-09T18:59:42Z

How many utilities you see provide 3-4 level subcmds ? This library has to support that use case so we cannot make choices assuming a certain depth. urfave/cli is more akin to a platform than a cli library. It allows lots of configurations so that people can achieve what they want from a cli library. Just because something is good for one user doesnt mean that others want it. Any changes have to be backward compatible(within major versions) and if it is an optional feature it needs to turned off by default. Interested users should be able to enable it as needed. I have thought about this in the past and felt there needs to be a "personality" setting for the library which configures a set of defaults automatically to provide a given behavior. As an example we would provide say 'ls' personality or 'git' personality and the cli would behave similar to what those utilities do in terms of flags/subcmds in terms of how options are parsed(reorder args) and help output. Additionally users would be able to plugin their own personality engine which would define behaviors expected of the platform.

fiatjaf · 2024-07-12T21:25:20Z

I made it work for subcommands -- now it seems to be working fine in all my manual tests -- and made the behavior optional.

But some test in the suite and I'm having trouble understanding which. When I run go test I get a thousand lines of output from command calls. Please help me!

dearchap · 2024-07-13T00:08:02Z

command.go

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


Suggested change

func argIsFlag(commandFlags []Flag, arg string) (isFlag bool, isBooleanFlag bool) {

func *c *Command) argIsFlag(arg string) (isFlag bool, isBooleanFlag bool) {

dearchap · 2024-07-13T00:10:22Z

command.go

+		return false, false
+	}
+	// this line turns `--flag` into `flag`
+	if strings.HasPrefix(arg, "--") {


Suggested change

if strings.HasPrefix(arg, "--") {

arg, _ = strings.CutPrefix(arg, "--")

dearchap · 2024-07-13T00:10:59Z

command.go

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


Suggested change

if strings.HasPrefix(arg, "-") {

arg, _ = strings.CutPrefix(arg, "-")

I'm curious why, is this more efficient?

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

dearchap · 2024-07-13T02:04:23Z

command.go

+	// 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 {


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

dearchap · 2024-07-13T02:05:29Z

command.go

@@ -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"`


Suggested change

AllowFlagsAfterArguments bool `json:"AllowFlagsAfterArguments"`

AllowFlagsAfterArguments bool `json:"allowFlagsAfterArguments"`

dearchap · 2024-07-13T02:12:05Z

command.go

+			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 {


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

dearchap · 2024-07-13T02:16:01Z

command.go

+			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


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

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.

dearchap · 2024-07-13T02:17:33Z

@fiatjaf Can you add some test cases for this first ? If you want to test locally you can do

go test -v -failfast

that should give you test which failed immediately

dearchap · 2024-07-13T02:19:44Z

command.go

+	for _, flag := range commandFlags {
+		for _, key := range flag.Names() {
+			if key == arg {
+				_, isBooleanFlag = flag.(*BoolFlag)


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

fiatjaf · 2024-07-13T14:08:44Z

Working on tests now.

dearchap · 2024-07-13T22:02:43Z

@fiatjaf One more thing is to check the behavior when ShellCompletion is enabled. Pressing TAB and having the reorder, now sure how that might work.

decentral1se · 2024-07-14T12:45:15Z

One thing I ran into on manual testing latest changes. One example to compare from another tool. You can do hugo -h and hugo -h serve and still get the same main command help output. In the current changes, the -h would get reordered to hugo serve -h which I think will cause confusion. I think this might relate to the discussion in #1950 where reordering should be "local first"?

fiatjaf · 2024-07-14T15:41:56Z

OK, I'm giving up, sorry.

dearchap · 2024-07-14T20:52:17Z

@fiatjaf if you'd like us to add this feature please keep the branch open. We can try picking it up later . As you can see it's not that trivial to implement . For a particular use case it's easy but to have it work for all users for different use cases is extremely difficult. That's something we've always struggled with

abitrolly · 2024-07-16T08:32:02Z

We can add "Unsolvable challenges" chapter with examples and clarification to documentation. That will be useful for people from all CLI frameworks to tackle this problem without spending extra too much time on researching these tangled bits.

fiatjaf · 2024-07-16T10:53:29Z

This "unsolvable challenge" is implemented in every other CLI framework, it's the number 1 requested feature according to your own link above, and this PR implements it perfectly as far as I know (except for the autocomplete thing which I have no idea of how it works, never managed to get it working on my apps).

Honestly, I don't understand why you're so dismissive of such a good feature that everybody wants. It would be a good idea to even sacrifice some of the other features just to get this, but I don't even think that is necessary. I just hope you take a look at it at some point in the future.

abitrolly · 2024-07-17T13:23:25Z

@fiatjaf hey, I am not dismissive. Now that you've created the issue, the link https://github.com/urfave/cli/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc correctly lists it as the most upvoted feature (with my upvote included), but before that there was no data to back up you claim. )

I can confirm that in Python argparse flags can come after arguments by default.

$ cat postarg.py
import argparse

parser = argparse.ArgumentParser()

parser.add_argument('filename')           # positional argument
parser.add_argument('-c', '--count')      # option that takes a value
parser.add_argument('-v', '--verbose',
                    action='store_true')  # on/off flag

args = parser.parse_args()
print(args.filename, args.count, args.verbose)

$ python postarg.py -v filename -c 4
filename 4 True
$ python postarg.py -c4 -v filename 
filename 4 True

I personally ~~hate~~ don't like that go binary itself has very awful command help interface.

$ go mod -h
go mod: unknown command
Run 'go help mod' for usage.

It could be much better if go supported -h option, which could be stuffed at the end.

ellistarn · 2024-09-27T21:18:16Z

👍 for this feature!

I decided to try out urfave/cli, as it had some nice idiomatic patterns compared to cobra. The lack of this feature is a deciding factor that will keep me using cobra. I will always favor the ergonomics of my customers rather than my own ergonomics as a developer.

dearchap · 2024-10-06T00:20:08Z

@fiatjaf I've implemented this feature in #1977 so as to keep backward compatibility and work with autocomplete. I am closing this. Can you review that PR ? Much appreciated

dearchap · 2024-10-14T01:15:59Z

@fiatjaf I have a potential fix at github.com:dearchap/cli/pos_args_flags_mix . Want to try it out ?

fiatjaf · 2024-10-14T14:55:51Z

Seems to work pretty well for me now!

fiatjaf requested a review from a team as a code owner June 26, 2024 02:19

fiatjaf force-pushed the flags-after-args branch from 2d6a54d to 8b84935 Compare June 30, 2024 20:30

decentral1se mentioned this pull request Jul 9, 2024

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

Closed

fiatjaf mentioned this pull request Jul 9, 2024

Allow flags to come after arguments #1950

Open

3 tasks

fiatjaf added 2 commits July 12, 2024 17:51

reorder flags so they can come after arguments.

b4605ff

fix it for subcommands and make it optional.

3a8b028

fiatjaf force-pushed the flags-after-args branch from e6aa7ca to 3a8b028 Compare July 12, 2024 21:21

dearchap reviewed Jul 13, 2024

View reviewed changes

address comments.

380173a

fix failing test.

ebd1e3e

fiatjaf closed this Jul 14, 2024

fix UseShortOptionHandling case on reorderFlags.

bb03655

fiatjaf reopened this Jul 14, 2024

dearchap closed this Oct 6, 2024

dearchap reopened this Oct 6, 2024

dearchap mentioned this pull request Oct 14, 2024

Fix:(issue_976) All positional args and flags to be mixed in any order #1987

Open

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

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

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

	AllowFlagsAfterArguments bool `json:"AllowFlagsAfterArguments"`
	AllowFlagsAfterArguments bool `json:"allowFlagsAfterArguments"`

reorder flags so they can come after arguments #1928

Are you sure you want to change the base?

reorder flags so they can come after arguments #1928

Conversation

fiatjaf commented Jun 26, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Testing

skelouse commented Jun 26, 2024

linrl3 commented Jul 4, 2024

fiatjaf commented Jul 4, 2024

dearchap commented Jul 4, 2024 • edited Loading

fiatjaf commented Jul 4, 2024 • edited Loading

joshfrench commented Jul 4, 2024

dearchap commented Jul 4, 2024

fiatjaf commented Jul 4, 2024

dearchap commented Jul 4, 2024

decentral1se commented Jul 8, 2024

abitrolly commented Jul 9, 2024

decentral1se commented Jul 9, 2024 • edited Loading

fiatjaf commented Jul 9, 2024

XANi commented Jul 9, 2024

decentral1se commented Jul 9, 2024 • edited Loading

fiatjaf commented Jul 9, 2024

dearchap commented Jul 9, 2024

fiatjaf commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dearchap commented Jul 13, 2024

Choose a reason for hiding this comment

fiatjaf commented Jul 13, 2024

dearchap commented Jul 13, 2024

decentral1se commented Jul 14, 2024

fiatjaf commented Jul 14, 2024 • edited Loading

dearchap commented Jul 14, 2024

abitrolly commented Jul 16, 2024

fiatjaf commented Jul 16, 2024 • edited Loading

abitrolly commented Jul 17, 2024

ellistarn commented Sep 27, 2024

dearchap commented Oct 6, 2024

dearchap commented Oct 14, 2024

fiatjaf commented Oct 14, 2024

dearchap commented Jul 4, 2024 •

edited

Loading

fiatjaf commented Jul 4, 2024 •

edited

Loading

decentral1se commented Jul 9, 2024 •

edited

Loading

decentral1se commented Jul 9, 2024 •

edited

Loading

fiatjaf commented Jul 14, 2024 •

edited

Loading

fiatjaf commented Jul 16, 2024 •

edited

Loading