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

fix: stop automatic sorting for --help #1430

Merged
merged 6 commits into from Oct 9, 2022

Conversation

FGYFFFF
Copy link
Contributor

@FGYFFFF FGYFFFF commented Jul 2, 2022

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 FGYFFFF requested a review from a team as a code owner July 2, 2022 10:22
@FGYFFFF FGYFFFF force-pushed the fix/stop_auto_sort_for_help branch 2 times, most recently from 45c0272 to e27277f Compare July 2, 2022 10:46
meatballhat
meatballhat previously approved these changes Jul 9, 2022
Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

@FGYFFFF Thank you! Are you able to add some tests around this, too? If not, I'm happy to pick up the work.

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Jul 11, 2022

@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
Copy link
Member

@meatballhat I will add unit tests later.

Did you mean you will be adding tests as part of this PR?

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Jul 20, 2022

@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?

@meatballhat
Copy link
Member

@FGYFFFF Adding a test to this PR, please 👍🏼 😁

@gzigzigzeo
Copy link

gzigzigzeo commented Aug 9, 2022

Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling

cli/category.go

Line 163 in 0c9527f

sort.Strings(vfNames)
and

cli/category.go

Line 126 in 0c9527f

sort.Strings(catNames)
worked.

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Aug 10, 2022

Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling

cli/category.go

Line 163 in 0c9527f

sort.Strings(vfNames)

and

cli/category.go

Line 126 in 0c9527f

sort.Strings(catNames)

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".

@gzigzigzeo
Copy link

Tested this PR with my project. Unfortunately, it does not fix the original issue. Disabling

cli/category.go

Line 163 in 0c9527f

sort.Strings(vfNames)

and

cli/category.go

Line 126 in 0c9527f

sort.Strings(catNames)

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.

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Aug 10, 2022

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.

@gzigzigzeo
Copy link

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.

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Aug 10, 2022

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.

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

@FGYFFFF
Copy link
Contributor Author

FGYFFFF commented Aug 11, 2022

@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.

@dearchap
Copy link
Contributor

@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.

@ixje
Copy link

ixje commented Aug 24, 2022

Interested in this as well

@dearchap dearchap added the area/v2 relates to / is being considered for v2 label Sep 26, 2022
@meatballhat meatballhat added status/waiting-for-response Waiting for response from original requester and removed status/user-feedback-required labels Oct 2, 2022
@dearchap dearchap dismissed stale reviews from ghost and meatballhat via 02613e5 October 7, 2022 01:53
@dearchap
Copy link
Contributor

dearchap commented Oct 7, 2022

@FGYFFFF @meatballhat I fixed the code to ignore empty category and also fixed the tests for this.

@dearchap dearchap removed the status/waiting-for-response Waiting for response from original requester label Oct 7, 2022
@dearchap
Copy link
Contributor

dearchap commented Oct 8, 2022

@meatballhat Can you take a look at test-docs ?

@meatballhat
Copy link
Member

@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": ["&#45;&#45;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

@dearchap
Copy link
Contributor

dearchap commented Oct 9, 2022

@meatballhat Update the flags.md. All tests passed.

@dearchap dearchap merged commit f37b9d9 into urfave:main Oct 9, 2022
another-rex added a commit to google/osv.dev that referenced this pull request Oct 13, 2022
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/g-rath/osv-detector](https://togithub.com/g-rath/osv-detector)
| require | minor | `v0.7.2` -> `v0.8.0` |
| [github.com/urfave/cli/v2](https://togithub.com/urfave/cli) | require
| minor | `v2.17.1` -> `v2.19.2` |
| [golang.org/x/crypto](https://togithub.com/golang/crypto) | require |
digest | `4161e89` -> `56aed06` |
| [golang.org/x/exp](https://togithub.com/golang/exp) | require | digest
| `b9f4876` -> `4de253d` |

---

### Release Notes

<details>
<summary>g-rath/osv-detector</summary>

###
[`v0.8.0`](https://togithub.com/G-Rath/osv-detector/releases/tag/v0.8.0)

[Compare
Source](https://togithub.com/g-rath/osv-detector/compare/v0.7.2...v0.8.0)

#### What's Changed

- support parsing `poetry.lock`, for Python
([G-Rath/osv-detector#156)
- support parsing `pubspec.lock`, for Dart
([G-Rath/osv-detector#159)

**Full Changelog**:
G-Rath/osv-detector@v0.7.2...v0.8.0

</details>

<details>
<summary>urfave/cli</summary>

### [`v2.19.2`](https://togithub.com/urfave/cli/releases/tag/v2.19.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.19.1...v2.19.2)

#### What's Changed

- fix: stop automatic sorting for --help by
[@&#8203;FGYFFFF](https://togithub.com/FGYFFFF) in
[urfave/cli#1430

#### New Contributors

- [@&#8203;FGYFFFF](https://togithub.com/FGYFFFF) made their first
contribution in
[urfave/cli#1430

**Full Changelog**:
urfave/cli@v2.19.1...v2.19.2

### [`v2.19.1`](https://togithub.com/urfave/cli/releases/tag/v2.19.1)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.19.0...v2.19.1)

#### What's Changed

- Fix:(issue\_1500). Fix slice flag value duplication issue by
[@&#8203;dearchap](https://togithub.com/dearchap) in
[urfave/cli#1502

**Full Changelog**:
urfave/cli@v2.19.0...v2.19.1

### [`v2.19.0`](https://togithub.com/urfave/cli/releases/tag/v2.19.0)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.2...v2.19.0)

#### What's Changed

- Fix:(issue\_1505) Fix flag alignment in help by
[@&#8203;dearchap](https://togithub.com/dearchap) in
[urfave/cli#1506

**Full Changelog**:
urfave/cli@v2.18.2...v2.19.0

### [`v2.18.2`](https://togithub.com/urfave/cli/releases/tag/v2.18.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.1...v2.18.2)

#### What's Changed

- Configure GenericFlag's Destination type as struct not pointer by
[@&#8203;nkuba](https://togithub.com/nkuba) in
[urfave/cli#1442

#### New Contributors

- [@&#8203;nkuba](https://togithub.com/nkuba) made their first
contribution in
[urfave/cli#1442

**Full Changelog**:
urfave/cli@v2.18.1...v2.18.2

### [`v2.18.1`](https://togithub.com/urfave/cli/releases/tag/v2.18.1)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.0...v2.18.1)

#### What's Changed

- Ensure "generate" step runs in CI prior to diff check by
[@&#8203;meatballhat](https://togithub.com/meatballhat) in
[urfave/cli#1504

**Full Changelog**:
urfave/cli@v2.18.0...v2.18.1

### [`v2.18.0`](https://togithub.com/urfave/cli/releases/tag/v2.18.0)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.17.2...v2.18.0)

#### What's Changed

- Call FlagStringer in String() method of slice flags by
[@&#8203;fjl](https://togithub.com/fjl) in
[urfave/cli#1508

#### New Contributors

- [@&#8203;fjl](https://togithub.com/fjl) made their first contribution
in
[urfave/cli#1508

**Full Changelog**:
urfave/cli@v2.17.2...v2.18.0

### [`v2.17.2`](https://togithub.com/urfave/cli/releases/tag/v2.17.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.17.1...v2.17.2)

#### What's Changed

- Remove nonexistent phony targets by
[@&#8203;meatballhat](https://togithub.com/meatballhat) in
[urfave/cli#1503
- wrap: Avoid trailing whitespace for empty lines by
[@&#8203;abitrolly](https://togithub.com/abitrolly) in
[urfave/cli#1513

#### New Contributors

- [@&#8203;abitrolly](https://togithub.com/abitrolly) made their first
contribution in
[urfave/cli#1513

**Full Changelog**:
urfave/cli@v2.17.1...v2.17.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on monday" in timezone
Australia/Sydney, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/google/osv.dev).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMjIuMyIsInVwZGF0ZWRJblZlciI6IjMyLjIzNC4yIn0=-->

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flags no longer sort by definition order
5 participants