Skip to content

Conversation

@Poweranimal
Copy link

@Poweranimal Poweranimal commented Feb 4, 2021

A cli flag used for enums.
The flag displays all possible enum variations in the cli's help message and verifies the provided input value.

What type of PR is this?

  • feature

What this PR does / why we need it:

This PR adds an enum flag implementation.

Special notes for your reviewer:

I'm open for any suggestion to fulfill the job of supporting an enum flag better.

Example

type LogFormat int32

const (
    LogFormatJSON LogFormat = iota
    LogFormatText
)

type LogMode in32

const (
    LogModeNoop LogMode = iota
    LogModeStdout
)

var logModeMap = map[LogMode]string{
    LogModeNoop: "noop",
    LogModeStdout: "stdout",
}

func (m LogMode) String() string {
    return logModeMap[m]
}

var (
    destFormat LogFormat
    destLevel string
)

var LogFormatFlag = &cli.ChoiceFlag{
    Name:         "log-format",
    Value:          LogFormatJSON,
    Choice:        cli.NewChoice(map[string]interface{}{
        "json": LogFormatJSON,
        "text":  LogFormatText,
    },
    Destination: &destFormat,
    EnvVars:      []string{"LOG_FORMAT"},
    Usage:         "The output formatting of the logger.",
    Required:     false,
}

var LogModeFlag = &cli.ChoiceFlag{
    Name:         "log-mode",
    Value:          LogModeStdout,
    Choice:        cli.NewStringerChoice(LogModeNoop, LogModeStdout),
    EnvVars:      []string{"LOG_MODE"},
    Usage:         "The log mode of the logger.",
}

var LogLevelFlag = &cli.ChoiceFlag{
    Name:         "log-level",
    Value:          "info",
    Choice:        cli.NewStringChoice("debug", "info", "warn", "error"),
    Destination: &destLevel,
    EnvVars:      []string{"LOG_LEVEL"},
    Usage:         "The log level of the logger.",
    Required:     false,
}

Release Notes


Added `cli.ChoiceFlag` for enum support.

TODOS

  • Typed destination
  • Add tests
  • Add README

@Poweranimal Poweranimal requested a review from a team as a code owner February 4, 2021 19:06
@Poweranimal Poweranimal requested review from asahasrabuddhe and coilysiren and removed request for a team February 4, 2021 19:06
Copy link
Contributor

@dearchap dearchap left a comment

Choose a reason for hiding this comment

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

Great good.

@dearchap
Copy link
Contributor

dearchap commented Feb 7, 2021

I just realised. This is more generic than enums. You could implement an option of choices for strings, ints, etc for this. Maybe rename this to ChoiceFlag ?

added EnumByName to Context -> Context.Enum
renamed flag_enum.go to flag_choice.go
@Poweranimal
Copy link
Author

I just realised. This is more generic than enums. You could implement an option of choices for strings, ints, etc for this. Maybe rename this to ChoiceFlag ?

Yeah, that sounds better.
I renamed 'enum' to 'choice'

@Poweranimal
Copy link
Author

@dearchap Regarding adding a Destination.
This seems a bit more complicated.
As of now, this package uses for destination the flag package of the golang stdlib.
It doesn't play well with the ChoiceHolder.
Let me see, if I figure out how one can support this...

poweranimal added 2 commits February 7, 2021 16:23
improved ChoiceFlag api
@Poweranimal
Copy link
Author

I added destination to the ChoiceFlag and improved its API.

Copy link
Contributor

@dearchap dearchap left a comment

Choose a reason for hiding this comment

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

You should also write some unit tests for this for completion.

@Poweranimal
Copy link
Author

Poweranimal commented Feb 8, 2021

I did some changes to the ChoiceFlag;

  • Replaced ChoiceHolder with interface{}
  • Renamed ChoiceDecoder to Choice
  • Added ToString to Choice
  • Added helper Choice constructors: NewChoice and NewStringChoice

Copy link

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

We might also want to update the README.md with usage example and documenting the availabiltiy of this param type.

@coilysiren coilysiren requested review from a team and removed request for asahasrabuddhe and coilysiren April 23, 2021 19:44
@marcofranssen
Copy link

What is the state of this PR? Seems only blocked on some missing code coverage for the new feature. @Poweranimal is that something you could finalize?

@Poweranimal
Copy link
Author

Hi @marcofranssen
I got quite demotivated to continue due to the lack of engagement by the maintainers.

Thus, I got the impression that this project is EOL and doesn't welcome pr's anymore.

@abdennour
Copy link

@Poweranimal , you did great job. I hope better response from maintainers

@meatballhat meatballhat added status/stale stale due to the age of it's last update and removed status/stale stale due to the age of it's last update labels Apr 21, 2022
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 labels Apr 21, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the title added enum flag Add ChoiceFlag Apr 21, 2022
@meatballhat meatballhat changed the base branch from master to main April 21, 2022 22:05
@meatballhat
Copy link
Member

@Poweranimal Hello and sorry for the long delay 😭 Please come back! This whole global pandemic thing and various maintainers' life adventures have seriously impacted responsiveness, but we're trying to get stuff flowing again now.

@meatballhat
Copy link
Member

Connected to #602

@meatballhat meatballhat added the status/conflicts contains merge conflicts label May 8, 2022
@jalavosus
Copy link
Contributor

jalavosus commented Jun 26, 2022

@Poweranimal if you could rebase this onto the current main branch, I'd love to review this and get it merged.

Edit to add: rebase and add some tests to this, both for codecov happiness reasons as well as that having tests is usually a good thing.

Thanks! :D

@Poweranimal
Copy link
Author

Hi @jalavosus ,
I’ve quit using this repo for quite l long time now.
Please feel free to to reuse my branch as you like.

@dearchap
Copy link
Contributor

@meatballhat I think this is a good feature but there are many ways to go about it. Either the ChoiceFlag as given here or a Choice interface can be added to each flag(IntFlag, StringFlag etc) in the generated code. I am inkling towards the latter approach. I can work on it if you think its the right direction.

@meatballhat
Copy link
Member

@dearchap I would love to see what you have in mind for a Choice interface!

@dearchap
Copy link
Contributor

dearchap commented Oct 6, 2022

This approach has been considered but dropped due to favoring a different mechanism using a validation API. That would allow ALL flags to use validation code instead of creating a new Flag

@dearchap dearchap closed this Oct 6, 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 status/conflicts contains merge conflicts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants