cli
cli copied to clipboard
fix: stop automatic sorting for --help
What type of PR is this?
- bug
What this PR does / why we need it:
Wrongly added catecory "" to each flag automatically, causing --help to sort automatically
Which issue(s) this PR fixes:
Fixes #1420
Special notes for your reviewer:
Testing
Release Notes
Add category only for flags that declare "category"
@FGYFFFF Thank you! Are you able to add some tests around this, too? If not, I'm happy to pick up the work. @meatballhat I will add unit tests later.
@meatballhat I will add unit tests later.
Did you mean you will be adding tests as part of this PR?
@meatballhat I will add unit tests later.
Did you mean you will be adding tests as part of this PR?
Yes! Do I add a test to this pr or mention another pr?
@FGYFFFF Adding a test to this PR, please 👍🏼 😁
Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L163 and https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L126 worked.
Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling
https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L163
and https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L126
worked.
If your flag does not define "category", then the displayed flags will be in the order you define them. If your flag defines "category" then the flags will be grouped and sorted according to "category".
Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L163
and https://github.com/urfave/cli/blob/0c9527f4b7f5618832c90d8b6c1ead4b7b1e2a80/category.go#L126
worked.
If your flag does not define "category", then the displayed flags will be in the order you define them. If your flag defines "category" then the flags will be grouped and sorted according to "category".
I've tried to reproduce the original issue using the following steps:
go.mod points to this commit:
replace github.com/urfave/cli/v2 => github.com/FGYFFFF/cli/v2 v2.10.4-0.20220702104559-e27277fa75de
Flags are defined in the following order: host
, port
, path
, health-path
.
Here's what I see on -h
:
--health-path value HTTP health endpoint path (default: /health) [$ANYCABLE_HEALTH_PATH]
--help, -h show help (default: false)
--host value Server host (default: localhost) [$ANYCABLE_HOST]
--path value WebSocket endpoint path (you can specify multiple paths using comma as separator) (default: /cable) [$ANYCABLE_PATH]
--port value Server port (default: 8080) [$PORT, $ANYCABLE_PORT]
--version, -v print the version (default: false)
Here's what I see when I comment line 179 in category.go
(sort.Strings
in *defaultVisibleFlagCategory Flags()
):
--host value Server host (default: localhost) [$ANYCABLE_HOST]
--port value Server port (default: 8080) [$PORT, $ANYCABLE_PORT]
--path value WebSocket endpoint path (you can specify multiple paths using comma as separator) (default: /cable) [$ANYCABLE_PATH]
--health-path value HTTP health endpoint path (default: /health) [$ANYCABLE_HEALTH_PATH]
--help, -h show help (default: false)
--version, -v print the version (default: false)
Works the same with either category defined or blank.
I don't think we're talking about a problem. If you declare a flag that defines a "category", then the flags under that "category" grouping need to be sorted. For example:
idlFlag := cli.StringSliceFlag{Name: "idl", Usage: "Specify the IDL file path. (.thrift or .proto)"}
moduleFlag := cli.StringFlag{Name: "module", Aliases: []string{"mod"}, Usage: "Specify the Go module name to generate go.mod.", Destination: &globalArgs.Gomod, **Category**:"fgy"}
serviceNameFlag := cli.StringFlag{Name: "service", Usage: "Specify the service name.", Destination: &globalArgs.ServiceName}
When "moduleFlag" declares "category", all flags will be sorted for below
OPTIONS:
--idl value Specify the IDL file path. (.thrift or .proto) (accepts multiple inputs)
--service value Specify the service name.
fgy
--module value, --mod value Specify the Go module name to generate go.mod.
But if you don't specify "category", then all flags should not be sorted:
idlFlag := cli.StringSliceFlag{Name: "idl", Usage: "Specify the IDL file path. (.thrift or .proto)"}
moduleFlag := cli.StringFlag{Name: "module", Aliases: []string{"mod"}, Usage: "Specify the Go module name to generate go.mod.", Destination: &globalArgs.Gomod}
serviceNameFlag := cli.StringFlag{Name: "service", Usage: "Specify the service name.", Destination: &globalArgs.ServiceName}
The flags will be displayed in the defined order:
OPTIONS:
--idl value Specify the IDL file path. (.thrift or .proto) (accepts multiple inputs)
--module value, --mod value Specify the Go module name to generate go.mod.
--service value Specify the service name.
But if you don't specify "category", then all flags should not be sorted:
That is correct. I have defined my flags in the following order: host
, port
, path
, health-path
. Category is empty. I see them in the help output sorted alphabetically: health-path
, host
, path
, port
.
Besides that, I am not so sure that it's a good idea to sort flags within a category. I'd like to control that order somehow in my app. Having a flag which controls this behaviour would be great.
But if you don't specify "category", then all flags should not be sorted:
idlFlag := cli.StringSliceFlag{Name: "idl", Usage: "Specify the IDL file path. (.thrift or .proto)"} moduleFlag := cli.StringFlag{Name: "module", Aliases: []string{"mod"}, Usage: "Specify the Go module name to generate go.mod.", Destination: &globalArgs.Gomod} serviceNameFlag := cli.StringFlag{Name: "service", Usage: "Specify the service name.", Destination: &globalArgs.ServiceName}
The flags will be displayed in the defined order:
OPTIONS: --idl value Specify the IDL file path. (.thrift or .proto) (accepts multiple inputs) --service value Specify the service name. --module value, --mod value Specify the Go module name to generate go.mod.
In my understanding, this example is not exactly correct: flags are defined in the following order: idl
, module
, service
, but the help order is idl
, service
, module
.
But if you don't specify "category", then all flags should not be sorted:
That is correct. I have defined my flags in the following order:
host
,port
,path
,health-path
. Category is empty. I see them in the help output sorted alphabetically:health-path
,host
,path
,port
.Besides that, I am not so sure that it's a good idea to sort flags within a category. I'd like to control that order somehow in my app. Having a flag which controls this behaviour would be great.
But if you don't specify "category", then all flags should not be sorted:
idlFlag := cli.StringSliceFlag{Name: "idl", Usage: "Specify the IDL file path. (.thrift or .proto)"} moduleFlag := cli.StringFlag{Name: "module", Aliases: []string{"mod"}, Usage: "Specify the Go module name to generate go.mod.", Destination: &globalArgs.Gomod} serviceNameFlag := cli.StringFlag{Name: "service", Usage: "Specify the service name.", Destination: &globalArgs.ServiceName}
The flags will be displayed in the defined order:
OPTIONS: --idl value Specify the IDL file path. (.thrift or .proto) (accepts multiple inputs) --service value Specify the service name. --module value, --mod value Specify the Go module name to generate go.mod.
In my understanding, this example is not exactly correct: flags are defined in the following order:
idl
,module
,service
, but the help order isidl
,service
,module
.
Yes, I agree with you: "category" should not be sorted automatically. But you can look at the implementation of category. The implementation of category uses "map", which results in a different order each time the map is traversed, so category is sorted once by default
@meatballhat Hey, friend!!! I've thought about this piece of code and I don't think I need to write unit tests, can I merge it in? My code currently relies on this feature.
@FGYFFFF Without unit tests we cannot approve this PR since this is a behavioral change. If this is expected behaviour this needs to be in a unit test so that we can confirm that some other change doesnt break this down the road.
Interested in this as well
@FGYFFFF @meatballhat I fixed the code to ignore empty category and also fixed the tests for this.
@meatballhat Can you take a look at test-docs ?
@FGYFFFF @dearchap From what I can tell, the failure in the test-docs
step is legitimate. This is the diff of what I have locally to fix it:
diff --git a/docs/v2/examples/flags.md b/docs/v2/examples/flags.md
index dbb41ea..44c643a 100644
--- a/docs/v2/examples/flags.md
+++ b/docs/v2/examples/flags.md
@@ -239,7 +239,7 @@ For example this:
<!-- {
"args": ["--help"],
- "output": ".*Load configuration from FILE\n.*\n.*Language for the greeting.*"
+ "output": ".*Load configuration from FILE\n.*Language for the greeting.*"
} -->
```go
package main
@meatballhat Update the flags.md. All tests passed.