cli icon indicating copy to clipboard operation
cli copied to clipboard

Add ChoiceFlag

Open Poweranimal opened this issue 4 years ago • 14 comments

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

  • [x] Typed destination
  • [ ] Add tests
  • [ ] Add README

Poweranimal avatar Feb 04 '21 19:02 Poweranimal

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 ?

dearchap avatar Feb 07 '21 13:02 dearchap

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 avatar Feb 07 '21 14:02 Poweranimal

@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 avatar Feb 07 '21 14:02 Poweranimal

I added destination to the ChoiceFlag and improved its API.

Poweranimal avatar Feb 07 '21 17:02 Poweranimal

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

Poweranimal avatar Feb 08 '21 12:02 Poweranimal

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?

marcofranssen avatar Jun 04 '21 07:06 marcofranssen

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.

Poweranimal avatar Jun 04 '21 08:06 Poweranimal

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

abdennour avatar Jul 23 '21 23:07 abdennour

@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 avatar Apr 21 '22 22:04 meatballhat

Connected to #602

meatballhat avatar Apr 22 '22 23:04 meatballhat

@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

jalavosus avatar Jun 26 '22 17:06 jalavosus

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.

Poweranimal avatar Jun 26 '22 19:06 Poweranimal

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

dearchap avatar Aug 13 '22 00:08 dearchap

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

meatballhat avatar Aug 14 '22 13:08 meatballhat

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 avatar Oct 06 '22 02:10 dearchap