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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag category support (#796) #1368

Merged
merged 17 commits into from May 22, 2022

Conversation

meatballhat
Copy link
Member

@meatballhat meatballhat commented Apr 22, 2022

What type of PR is this?

  • feature

What this PR does / why we need it:

See #796

Which issue(s) this PR fixes:

Closes #259

Special notes for your reviewer:

This PR is a best-effort porting of #796 so detailed criticism is very much appreciated 馃檱馃徏

Release Notes

add flag category support

michaeljs1990 and others added 8 commits Jan 27, 2019
This adds what I think needs to be done to support categories for flags
but we will see if that works. It also forces the scripts to use python2
since they blow up under python3 which is becoming the default python on
many linux systems.

Small fix to app_test as well so it conforms to the new Flag interface.
Let us use this repo via replace locally.
鈥1990/cli into michaeljs1990-add-flag-category-support
@meatballhat meatballhat requested a review from a team as a code owner Apr 22, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 22, 2022
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 labels Apr 22, 2022
category.go Outdated Show resolved Hide resolved
category.go Outdated Show resolved Hide resolved
dearchap
dearchap previously approved these changes Apr 29, 2022
@meatballhat meatballhat dismissed stale reviews from ghost and dearchap via ddac788 Apr 29, 2022
with internal maps instead of slices and slightly less public API
surface area
@meatballhat meatballhat requested a review from dearchap May 6, 2022
type CategorizableFlag interface {
VisibleFlag

GetCategory() string
Copy link
Contributor

@dearchap dearchap May 7, 2022

Choose a reason for hiding this comment

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

Rename to just Category(). golang doesnt recommend using GetXXX

Copy link
Member Author

@meatballhat meatballhat May 7, 2022

Choose a reason for hiding this comment

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

What about the conflict with the field name Category? 馃 I agree that GetXXX is generally bad style, but this is a language-level wart we're working around.

app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
catNames = append(catNames, name)
}

sort.Strings(catNames)
Copy link
Contributor

@dearchap dearchap May 7, 2022

Choose a reason for hiding this comment

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

isnt f.m already sorted ?

Copy link
Member Author

@meatballhat meatballhat May 7, 2022

Choose a reason for hiding this comment

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

IIUC ranging over a map has undefined sort order: https://go.dev/play/p/aR7JKg4ae8r Or did you mean something else? 馃

command.go Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
flag_bool.go Outdated
@@ -19,6 +19,7 @@ type BoolFlag struct {
DefaultText string
Destination *bool
HasBeenSet bool
Category string
Copy link
Contributor

@dearchap dearchap May 7, 2022

Choose a reason for hiding this comment

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

@meatballhat How would this tie-in with your generator code ?

Copy link
Member Author

@meatballhat meatballhat May 7, 2022

Choose a reason for hiding this comment

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

If all the flag types defined in this repo have Category fields, then it'd be part of the struct template. Or did you mean the potential merge conflict? (or something else?)

@meatballhat meatballhat dismissed a stale review via 21d435d May 7, 2022
@meatballhat meatballhat requested a review from dearchap May 7, 2022
@meatballhat meatballhat dismissed a stale review via a78717f May 19, 2022
@meatballhat meatballhat merged commit 9e65b4d into main May 22, 2022
13 checks passed
@meatballhat meatballhat deleted the michaeljs1990-add-flag-category-support branch May 22, 2022
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 kind/feature describes a code enhancement / feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants