From 4ecb53599b72bbe620e2417a3d6eed815a2e6f2a Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Tue, 10 Sep 2024 21:02:37 +1000 Subject: [PATCH] Make negatable flag name customisable (#439) * fix: Check if negatable duplicates another flag Add a check for flags with the `negatable` option if the negative flag conflicts with another tag, such as: Flag bool `negatable:""` NoFlag bool The flag `--no-flag` is ambiguous in this scenario. * feat: Make negatable flag name customisable Allow a value on the `negatable` tag to specify a flag name to use for negation instead of using `--no-` as the flag. e.g. Approve bool `default:"true",negatable:"deny"` This example will allow `--deny` to set the `Approve` field to false. --- README.md | 1 + build.go | 7 ++++ context.go | 6 ++-- help.go | 33 ++++++++----------- help_test.go | 3 ++ kong_test.go | 93 +++++++++++++++++++++++++++++++++++++++------------- negatable.go | 19 +++++++++++ tag.go | 15 ++++++--- 8 files changed, 128 insertions(+), 49 deletions(-) create mode 100644 negatable.go diff --git a/README.md b/README.md index 994da42..3936fde 100644 --- a/README.md +++ b/README.md @@ -571,6 +571,7 @@ Both can coexist with standard Tag parsing. | `optional:""` | If present, flag/arg is optional. | | `hidden:""` | If present, command or flag is hidden. | | `negatable:""` | If present on a `bool` field, supports prefixing a flag with `--no-` to invert the default value | +| `negatable:"X"` | If present on a `bool` field, supports `--X` to invert the default value | | `format:"X"` | Format for parsing input, if supported. | | `sep:"X"` | Separator for sequences (defaults to ","). May be `none` to disable splitting. | | `mapsep:"X"` | Separator for maps (defaults to ";"). May be `none` to disable splitting. | diff --git a/build.go b/build.go index 18048e0..7791c62 100644 --- a/build.go +++ b/build.go @@ -315,6 +315,13 @@ func buildField(k *Kong, node *Node, v reflect.Value, ft reflect.StructField, fv } seenFlags["-"+string(tag.Short)] = true } + if tag.Negatable != "" { + negFlag := negatableFlagName(value.Name, tag.Negatable) + if seenFlags[negFlag] { + return failField(v, ft, "duplicate negation flag %s", negFlag) + } + seenFlags[negFlag] = true + } flag := &Flag{ Value: value, Aliases: tag.Aliases, diff --git a/context.go b/context.go index ea0ec1c..c97aa84 100644 --- a/context.go +++ b/context.go @@ -710,13 +710,13 @@ func (c *Context) parseFlag(flags []*Flag, match string) (err error) { candidates = append(candidates, alias) } - neg := "--no-" + flag.Name - if !matched && !(match == neg && flag.Tag.Negatable) { + neg := negatableFlagName(flag.Name, flag.Tag.Negatable) + if !matched && match != neg { continue } // Found a matching flag. c.scan.Pop() - if match == neg && flag.Tag.Negatable { + if match == neg && flag.Tag.Negatable != "" { flag.Negated = true } err := flag.Parse(c.scan, c.getValue(flag.Value)) diff --git a/help.go b/help.go index cf5a912..26f355d 100644 --- a/help.go +++ b/help.go @@ -491,27 +491,22 @@ func formatFlag(haveShort bool, flag *Flag) string { name := flag.Name isBool := flag.IsBool() isCounter := flag.IsCounter() + + short := "" if flag.Short != 0 { - if isBool && flag.Tag.Negatable { - flagString += fmt.Sprintf("-%c, --[no-]%s", flag.Short, name) - } else { - flagString += fmt.Sprintf("-%c, --%s", flag.Short, name) - } - } else { - if isBool && flag.Tag.Negatable { - if haveShort { - flagString = fmt.Sprintf(" --[no-]%s", name) - } else { - flagString = fmt.Sprintf("--[no-]%s", name) - } - } else { - if haveShort { - flagString += fmt.Sprintf(" --%s", name) - } else { - flagString += fmt.Sprintf("--%s", name) - } - } + short = "-" + string(flag.Short) + ", " + } else if haveShort { + short = " " + } + + if isBool && flag.Tag.Negatable == negatableDefault { + name = "[no-]" + name + } else if isBool && flag.Tag.Negatable != "" { + name += "/" + flag.Tag.Negatable } + + flagString += fmt.Sprintf("%s--%s", short, name) + if !isBool && !isCounter { flagString += fmt.Sprintf("=%s", flag.FormatPlaceHolder()) } diff --git a/help_test.go b/help_test.go index e43a073..e680be2 100644 --- a/help_test.go +++ b/help_test.go @@ -71,6 +71,7 @@ func TestHelp(t *testing.T) { Map map[string]int `help:"A map of strings to ints."` Required bool `required help:"A required flag."` Sort bool `negatable short:"s" help:"Is sortable or not."` + Approve bool `negatable:"deny" help:"Approve or deny message."` One struct { Flag string `help:"Nested flag."` @@ -118,6 +119,7 @@ Flags: --map=KEY=VALUE;... A map of strings to ints. --required A required flag. -s, --[no-]sort Is sortable or not. + --approve/deny Approve or deny message. Commands: one --required [flags] @@ -159,6 +161,7 @@ Flags: --map=KEY=VALUE;... A map of strings to ints. --required A required flag. -s, --[no-]sort Is sortable or not. + --approve/deny Approve or deny message. --flag=STRING Nested flag under two. --required-two diff --git a/kong_test.go b/kong_test.go index c148962..7abf0ac 100644 --- a/kong_test.go +++ b/kong_test.go @@ -357,8 +357,9 @@ func TestTraceErrorPartiallySucceeds(t *testing.T) { } type commandWithNegatableFlag struct { - Flag bool `kong:"default='true',negatable"` - ran bool + Flag bool `kong:"default='true',negatable"` + Custom bool `kong:"default='true',negatable='standard'"` + ran bool } func (c *commandWithNegatableFlag) Run() error { @@ -368,34 +369,64 @@ func (c *commandWithNegatableFlag) Run() error { func TestNegatableFlag(t *testing.T) { tests := []struct { - name string - args []string - expected bool + name string + args []string + expectedFlag bool + expectedCustom bool }{ { - name: "no flag", - args: []string{"cmd"}, - expected: true, + name: "no flag", + args: []string{"cmd"}, + expectedFlag: true, + expectedCustom: true, }, { - name: "boolean flag", - args: []string{"cmd", "--flag"}, - expected: true, + name: "boolean flag", + args: []string{"cmd", "--flag"}, + expectedFlag: true, + expectedCustom: true, }, { - name: "inverted boolean flag", - args: []string{"cmd", "--flag=false"}, - expected: false, + name: "custom boolean flag", + args: []string{"cmd", "--custom"}, + expectedFlag: true, + expectedCustom: true, }, { - name: "negated boolean flag", - args: []string{"cmd", "--no-flag"}, - expected: false, + name: "inverted boolean flag", + args: []string{"cmd", "--flag=false"}, + expectedFlag: false, + expectedCustom: true, }, { - name: "inverted negated boolean flag", - args: []string{"cmd", "--no-flag=false"}, - expected: true, + name: "custom inverted boolean flag", + args: []string{"cmd", "--custom=false"}, + expectedFlag: true, + expectedCustom: false, + }, + { + name: "negated boolean flag", + args: []string{"cmd", "--no-flag"}, + expectedFlag: false, + expectedCustom: true, + }, + { + name: "custom negated boolean flag", + args: []string{"cmd", "--standard"}, + expectedFlag: true, + expectedCustom: false, + }, + { + name: "inverted negated boolean flag", + args: []string{"cmd", "--no-flag=false"}, + expectedFlag: true, + expectedCustom: true, + }, + { + name: "inverted custom negated boolean flag", + args: []string{"cmd", "--standard=false"}, + expectedFlag: true, + expectedCustom: true, }, } for _, tt := range tests { @@ -408,16 +439,34 @@ func TestNegatableFlag(t *testing.T) { p := mustNew(t, &cli) kctx, err := p.Parse(tt.args) assert.NoError(t, err) - assert.Equal(t, tt.expected, cli.Cmd.Flag) + assert.Equal(t, tt.expectedFlag, cli.Cmd.Flag) + assert.Equal(t, tt.expectedCustom, cli.Cmd.Custom) err = kctx.Run() assert.NoError(t, err) - assert.Equal(t, tt.expected, cli.Cmd.Flag) + assert.Equal(t, tt.expectedFlag, cli.Cmd.Flag) + assert.Equal(t, tt.expectedCustom, cli.Cmd.Custom) assert.True(t, cli.Cmd.ran) }) } } +func TestDuplicateNegatableLong(t *testing.T) { + cli2 := struct { + NoFlag bool + Flag bool `negatable:""` // negation duplicates NoFlag + }{} + _, err := kong.New(&cli2) + assert.EqualError(t, err, ".Flag: duplicate negation flag --no-flag") + + cli3 := struct { + One bool + Two bool `negatable:"one"` // negation duplicates Flag2 + }{} + _, err = kong.New(&cli3) + assert.EqualError(t, err, ".Two: duplicate negation flag --one") +} + func TestExistingNoFlag(t *testing.T) { var cli struct { Cmd struct { diff --git a/negatable.go b/negatable.go new file mode 100644 index 0000000..7d8902a --- /dev/null +++ b/negatable.go @@ -0,0 +1,19 @@ +package kong + +// negatableDefault is a placeholder value for the Negatable tag to indicate +// the negated flag is --no-. This is needed as at the time of +// parsing a tag, the field's flag name is not yet known. +const negatableDefault = "_" + +// negatableFlagName returns the name of the flag for a negatable field, or +// an empty string if the field is not negatable. +func negatableFlagName(name, negation string) string { + switch negation { + case "": + return "" + case negatableDefault: + return "--no-" + name + default: + return "--" + negation + } +} diff --git a/tag.go b/tag.go index 39be2e3..456f7ae 100644 --- a/tag.go +++ b/tag.go @@ -38,7 +38,7 @@ type Tag struct { EnvPrefix string Embed bool Aliases []string - Negatable bool + Negatable string Passthrough bool // Storage for all tag keys for arbitrary lookups. @@ -256,11 +256,16 @@ func hydrateTag(t *Tag, typ reflect.Type) error { //nolint: gocyclo t.Prefix = t.Get("prefix") t.EnvPrefix = t.Get("envprefix") t.Embed = t.Has("embed") - negatable := t.Has("negatable") - if negatable && !isBool && !isBoolPtr { - return fmt.Errorf("negatable can only be set on booleans") + if t.Has("negatable") { + if !isBool && !isBoolPtr { + return fmt.Errorf("negatable can only be set on booleans") + } + negatable := t.Get("negatable") + if negatable == "" { + negatable = negatableDefault // placeholder for default negation of --no- + } + t.Negatable = negatable } - t.Negatable = negatable aliases := t.Get("aliases") if len(aliases) > 0 { t.Aliases = append(t.Aliases, strings.FieldsFunc(aliases, tagSplitFn)...)