From 23a550cfd752b42324972b4ac63c98896bb1ef55 Mon Sep 17 00:00:00 2001 From: Sebastiaan Pasterkamp <26205277+SebastiaanPasterkamp@users.noreply.github.com> Date: Sat, 10 Dec 2022 13:55:09 +0100 Subject: [PATCH 1/5] fix(test): restore environment after parseWithEnv Make sure tests don't have side-effects potentially breaking other tests. --- parse_test.go | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/parse_test.go b/parse_test.go index 5d38306..3029760 100644 --- a/parse_test.go +++ b/parse_test.go @@ -44,12 +44,33 @@ func parseWithEnv(cmdline string, env []string, dest interface{}) (*Parser, erro } // split the environment vars + newEnv := map[string]string{} + oldEnv := map[string]string{} for _, s := range env { pos := strings.Index(s, "=") if pos == -1 { return nil, fmt.Errorf("missing equals sign in %q", s) } - err := os.Setenv(s[:pos], s[pos+1:]) + + if orig, ok := os.LookupEnv(s[:pos]); ok { + oldEnv[s[:pos]] = orig + } + + newEnv[s[:pos]] = s[pos+1:] + } + + defer func() { + for key, _ := range newEnv { + if orig, ok := oldEnv[key]; ok { + _ = os.Setenv(key, orig) + } else { + _ = os.Unsetenv(key) + } + } + }() + + for key, val := range newEnv { + err := os.Setenv(key, val) if err != nil { return nil, err } @@ -98,9 +119,9 @@ func TestInt(t *testing.T) { func TestHexOctBin(t *testing.T) { var args struct { - Hex int - Oct int - Bin int + Hex int + Oct int + Bin int Underscored int } err := parse("--hex 0xA --oct 0o10 --bin 0b101 --underscored 123_456", &args) From 00c1c8e7cd7fea1c9090c49b5c5173db5cc868f7 Mon Sep 17 00:00:00 2001 From: Sebastiaan Pasterkamp <26205277+SebastiaanPasterkamp@users.noreply.github.com> Date: Sat, 10 Dec 2022 14:18:00 +0100 Subject: [PATCH 2/5] chore(parse): make parse prep a method of command Avoid merging logic after parsing by making the parsing function a method of the command struct itself. This small refactoring is in preparation of parsing group structs later. --- parse.go | 64 +++++++++++++++++++++++++------------------------------- 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/parse.go b/parse.go index 6ac5e99..00e79b7 100644 --- a/parse.go +++ b/parse.go @@ -200,13 +200,13 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { } // process each of the destination values - for i, dest := range dests { + for _, dest := range dests { t := reflect.TypeOf(dest) if t.Kind() != reflect.Ptr { panic(fmt.Sprintf("%s is not a pointer (did you forget an ampersand?)", t)) } - cmd, err := cmdFromStruct(name, path{root: i}, t) + err := p.cmd.parseFieldsFromStructPointer(t) if err != nil { return nil, err } @@ -214,7 +214,7 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { // for backwards compatibility, add nonzero field values as defaults // this applies only to the top-level command, not to subcommands (this inconsistency // is the reason that this method for setting default values was deprecated) - for _, spec := range cmd.specs { + for _, spec := range p.cmd.specs { // get the value v := p.val(spec.dest) @@ -239,9 +239,6 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { } } - p.cmd.specs = append(p.cmd.specs, cmd.specs...) - p.cmd.subcommands = append(p.cmd.subcommands, cmd.subcommands...) - if dest, ok := dest.(Versioned); ok { p.version = dest.Version() } @@ -256,22 +253,20 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { return &p, nil } -func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { +// parseFieldsFromStructPointer ensures the destination structure is a pointer +// to a struct. This function should be called when parsing commands or +// subcommands as they can only be a struct pointer. +func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { // commands can only be created from pointers to structs if t.Kind() != reflect.Ptr { - return nil, fmt.Errorf("subcommands must be pointers to structs but %s is a %s", - dest, t.Kind()) + return fmt.Errorf("subcommands must be pointers to structs but %s is a %s", + cmd.dest, t.Kind()) } t = t.Elem() if t.Kind() != reflect.Struct { - return nil, fmt.Errorf("subcommands must be pointers to structs but %s is a pointer to %s", - dest, t.Kind()) - } - - cmd := command{ - name: name, - dest: dest, + return fmt.Errorf("subcommands must be pointers to structs but %s is a pointer to %s", + cmd.dest, t.Kind()) } var errs []string @@ -295,7 +290,7 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { } // duplicate the entire path to avoid slice overwrites - subdest := dest.Child(field) + subdest := cmd.dest.Child(field) spec := spec{ dest: subdest, field: field, @@ -308,7 +303,6 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { } // Look at the tag - var isSubcommand bool // tracks whether this field is a subcommand for _, key := range strings.Split(tag, ",") { if key == "" { continue @@ -348,24 +342,27 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { spec.env = strings.ToUpper(field.Name) } case key == "subcommand": + subCmd := command{ + name: value, + dest: subdest, + parent: cmd, + help: field.Tag.Get("help"), + } + cmd.subcommands = append(cmd.subcommands, &subCmd) + // decide on a name for the subcommand - cmdname := value - if cmdname == "" { - cmdname = strings.ToLower(field.Name) + if subCmd.name == "" { + subCmd.name = strings.ToLower(field.Name) } // parse the subcommand recursively - subcmd, err := cmdFromStruct(cmdname, subdest, field.Type) + err := subCmd.parseFieldsFromStructPointer(field.Type) if err != nil { errs = append(errs, err.Error()) return false } - subcmd.parent = &cmd - subcmd.help = field.Tag.Get("help") - - cmd.subcommands = append(cmd.subcommands, subcmd) - isSubcommand = true + return true default: errs = append(errs, fmt.Sprintf("unrecognized tag '%s' on field %s", key, tag)) return false @@ -381,11 +378,6 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { spec.placeholder = strings.ToUpper(spec.field.Name) } - // if this is a subcommand then we've done everything we need to do - if isSubcommand { - return false - } - // check whether this field is supported. It's good to do this here rather than // wait until ParseValue because it means that a program with invalid argument // fields will always fail regardless of whether the arguments it received @@ -440,7 +432,7 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { }) if len(errs) > 0 { - return nil, errors.New(strings.Join(errs, "\n")) + return errors.New(strings.Join(errs, "\n")) } // check that we don't have both positionals and subcommands @@ -451,10 +443,12 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { } } if hasPositional && len(cmd.subcommands) > 0 { - return nil, fmt.Errorf("%s cannot have both subcommands and positional arguments", dest) + return fmt.Errorf("%s cannot have both subcommands and positional arguments", + cmd.dest) + } - return &cmd, nil + return nil } // Parse processes the given command line option, storing the results in the field From ca8dc31b842c855b4d7ff29f68d7d62af594692a Mon Sep 17 00:00:00 2001 From: Sebastiaan Pasterkamp <26205277+SebastiaanPasterkamp@users.noreply.github.com> Date: Sat, 10 Dec 2022 14:59:23 +0100 Subject: [PATCH 3/5] fix: ensure Parser.val cannot be nil Used when adding values to subcommands as these are undefined pointers initially. This refactoring is in preparation of parsing group structs later. --- parse.go | 74 +++++++++++++++++++++++++++++++--------------- parse_test.go | 11 +++++++ subcommand_test.go | 9 ++---- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/parse.go b/parse.go index 00e79b7..0175977 100644 --- a/parse.go +++ b/parse.go @@ -216,27 +216,19 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { // is the reason that this method for setting default values was deprecated) for _, spec := range p.cmd.specs { // get the value - v := p.val(spec.dest) + defaultString, defaultValue, err := p.defaultVal(spec.dest) + if err != nil { + return nil, err + } // if the value is the "zero value" (e.g. nil pointer, empty struct) then ignore - if isZero(v) { + if defaultString == "" { continue } // store as a default - spec.defaultValue = v - - // we need a string to display in help text - // if MarshalText is implemented then use that - if m, ok := v.Interface().(encoding.TextMarshaler); ok { - s, err := m.MarshalText() - if err != nil { - return nil, fmt.Errorf("%v: error marshaling default value to string: %v", spec.dest, err) - } - spec.defaultString = string(s) - } else { - spec.defaultString = fmt.Sprintf("%v", v) - } + spec.defaultString = defaultString + spec.defaultValue = defaultValue } if dest, ok := dest.(Versioned); ok { @@ -575,11 +567,8 @@ func (p *Parser) process(args []string) error { return fmt.Errorf("invalid subcommand: %s", arg) } - // instantiate the field to point to a new struct - v := p.val(subcmd.dest) - if v.IsNil() { - v.Set(reflect.New(v.Type().Elem())) // we already checked that all subcommands are struct pointers - } + // ensure the command struct exists (is not a nil pointer) + p.val(subcmd.dest) // add the new options to the set of allowed options specs = append(specs, subcmd.specs...) @@ -743,20 +732,57 @@ func isFlag(s string) bool { return strings.HasPrefix(s, "-") && strings.TrimLeft(s, "-") != "" } -// val returns a reflect.Value corresponding to the current value for the -// given path -func (p *Parser) val(dest path) reflect.Value { +// defaultVal returns the string representation of the value at dest if it is +// reachable without traversing nil pointers, but only if it does not represent +// the default value for the type. +func (p *Parser) defaultVal(dest path) (string, reflect.Value, error) { v := p.roots[dest.root] for _, field := range dest.fields { if v.Kind() == reflect.Ptr { if v.IsNil() { - return reflect.Value{} + return "", v, nil } v = v.Elem() } v = v.FieldByIndex(field.Index) } + + if !v.IsValid() || isZero(v) { + return "", v, nil + } + + if defaultVal, ok := v.Interface().(encoding.TextMarshaler); ok { + str, err := defaultVal.MarshalText() + if err != nil { + return "", v, fmt.Errorf("%v: error marshaling default value to string: %w", dest, err) + } + return string(str), v, nil + } + + return fmt.Sprintf("%v", v), v, nil +} + +// val returns a reflect.Value corresponding to the current value for the +// given path initiating nil pointers in the path +func (p *Parser) val(dest path) reflect.Value { + v := p.roots[dest.root] + for _, field := range dest.fields { + if v.Kind() == reflect.Ptr { + if v.IsNil() { + v.Set(reflect.New(v.Type().Elem())) + } + v = v.Elem() + } + + v = v.FieldByIndex(field.Index) + } + + // Don't return a nil-pointer + if v.Kind() == reflect.Ptr && v.IsNil() { + v.Set(reflect.New(v.Type().Elem())) + } + return v } diff --git a/parse_test.go b/parse_test.go index 3029760..2fa3d92 100644 --- a/parse_test.go +++ b/parse_test.go @@ -930,6 +930,17 @@ func TestParserMustParse(t *testing.T) { } } +func TestNonPointerSubcommand(t *testing.T) { + var args struct { + Sub struct { + Foo string `arg:"env"` + } `arg:"subcommand"` + } + + _, err := NewParser(Config{IgnoreEnv: true}, &args) + require.Error(t, err, "subcommands must be pointers to structs but args.Sub is a struct") +} + type textUnmarshaler struct { val int } diff --git a/subcommand_test.go b/subcommand_test.go index 2c61dd3..c3909e6 100644 --- a/subcommand_test.go +++ b/subcommand_test.go @@ -1,7 +1,6 @@ package arg import ( - "reflect" "testing" "github.com/stretchr/testify/assert" @@ -402,12 +401,8 @@ func TestValForNilStruct(t *testing.T) { Sub *subcmd `arg:"subcommand"` } - p, err := NewParser(Config{}, &cmd) + _, err := NewParser(Config{}, &cmd) require.NoError(t, err) - typ := reflect.TypeOf(cmd) - subField, _ := typ.FieldByName("Sub") - - v := p.val(path{fields: []reflect.StructField{subField, subField}}) - assert.False(t, v.IsValid()) + require.Nil(t, cmd.Sub) } From 97d1ef3a3c24ac897e63939e24750e2eab28c4d4 Mon Sep 17 00:00:00 2001 From: Sebastiaan Pasterkamp <26205277+SebastiaanPasterkamp@users.noreply.github.com> Date: Sat, 10 Dec 2022 15:27:42 +0100 Subject: [PATCH 4/5] feat(opt groups): Add support for grouped options Nested structs are now suppored - both pointer and embedded. These nested structs can be presented as option groups with their own group name and help section. --- README.md | 64 +++++++++++ example_test.go | 49 ++++++++ parse.go | 87 +++++++++++++-- parse_test.go | 292 ++++++++++++++++++++++++++++++++++++++++++++++++ usage.go | 76 +++++++++---- usage_test.go | 199 ++++++++++++++++++++++++++++++++- 6 files changed, 730 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index f105b17..a891a61 100644 --- a/README.md +++ b/README.md @@ -583,6 +583,70 @@ if p.Subcommand() == nil { } ``` +### Option groups + +Option groups are a hybrid between subcommands and embedded structs. Option +groups create logical collections of related arguments with a help description, +and can be embedded in other groups and subcommands. Option groups can combine +configuration structs of multiple modules without requiring embedding. + +```go +type Repository struct { + URL string `arg:"--host" help:"URL of the repository" default:"docker.io"` + User string `arg:"--user,env:REPO_USERNAME" help:"username to connect as"` + Password string `arg:"--,env:REPO_PASSWORD" help:"password to connect with"` +} +type BuildCmd struct { + Context string + Tag string +} +type PushCmd struct { + Repo Repository `arg:"group:Repository" help:"Change the default registry to push to."` + Tag string `help:"Tag"` +} +var args struct { + Build *BuildCmd `arg:"subcommand:build"` + Push *PushCmd `arg:"subcommand:push"` + Quiet bool `arg:"-q" help:"Quiet"` // this flag is global to all subcommands +} + +arg.MustParse(&args) + +switch { +case args.Build != nil: + fmt.Printf("build %s as %q\n", args.Build.Context, args.Build.Tag) +case args.Push != nil: + fmt.Printf("push %q to %q\n", args.Push.Tag, args.Push.Repo.URL) +} +``` + +The push command help message would look like: + +```text +Usage: example push [--tag TAG] [--host HOST] [--user USER] + +Options: + --tag TAG Tag + +Repository options: + +Change the default registry to push to. + + --host HOST URL of the repository [default: docker.io] + --user USER username to connect as [env: REPO_USERNAME] + +Global options: + --quiet, -q Quiet + --help, -h display this help and exit +``` + +Some additional rules apply when working with option groups: +* The `group` tag can only be used with fields that are structs or pointers to structs. +* Specifying default values in nested struct pointers _always_ result in an initialized struct. +* Option groups may not contain any positionals. +* Option groups cannot contain sub-commands. +``` + ### API Documentation https://godoc.org/github.com/alexflint/go-arg diff --git a/example_test.go b/example_test.go index fd64777..2bd74a2 100644 --- a/example_test.go +++ b/example_test.go @@ -253,6 +253,55 @@ func Example_helpTextWithSubcommand() { // list list available items } +// This example shows the usage string generated by go-arg when using argument groups +func Example_helpTextWithGroups() { + os.Args = split("./example push --help") + + type Repository struct { + URL string `arg:"--host" help:"URL of the repository" default:"docker.io"` + User string `arg:"--user,env:REPO_USERNAME" help:"username to connect as"` + Password string `arg:"--,env:REPO_PASSWORD" help:"password to connect with"` + } + type BuildCmd struct { + Context string + Tag string + } + type PushCmd struct { + Repo Repository `arg:"group:Repository" help:"Change the default registry to push to."` + Tag string `help:"Tag"` + } + var args struct { + Build *BuildCmd `arg:"subcommand:build"` + Push *PushCmd `arg:"subcommand:push"` + Quiet bool `arg:"-q" help:"Quiet"` // this flag is global to all subcommands + } + + MustParse(&args) + + // This is only necessary when running inside golang's runnable example harness + osExit = func(int) {} + stdout = os.Stdout + + MustParse(&args) + + // output: + // Usage: example push [--tag TAG] [--host HOST] [--user USER] + // + // Options: + // --tag TAG Tag + // + // Repository options: + // + // Change the default registry to push to. + // + // --host HOST URL of the repository [default: docker.io] + // --user USER username to connect as [env: REPO_USERNAME] + // + // Global options: + // --quiet, -q Quiet + // --help, -h display this help and exit +} + // This example shows the usage string generated by go-arg when using subcommands func Example_helpTextWhenUsingSubcommand() { // These are the args you would pass in on the command line diff --git a/parse.go b/parse.go index 0175977..f2fc7bc 100644 --- a/parse.go +++ b/parse.go @@ -63,11 +63,23 @@ type command struct { name string help string dest path - specs []*spec + options []*spec subcommands []*command + groups []*command parent *command } +// specs gets all the specs from this command plus all nested option groups, +// recursively through descendants +func (cmd command) specs() []*spec { + var specs []*spec + specs = append(specs, cmd.options...) + for _, grpcmd := range cmd.groups { + specs = append(specs, grpcmd.specs()...) + } + return specs +} + // ErrHelp indicates that -h or --help were provided var ErrHelp = errors.New("help requested by user") @@ -206,7 +218,7 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { panic(fmt.Sprintf("%s is not a pointer (did you forget an ampersand?)", t)) } - err := p.cmd.parseFieldsFromStructPointer(t) + err := p.cmd.parseFieldsFromStructPointer(t, false) if err != nil { return nil, err } @@ -214,7 +226,7 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { // for backwards compatibility, add nonzero field values as defaults // this applies only to the top-level command, not to subcommands (this inconsistency // is the reason that this method for setting default values was deprecated) - for _, spec := range p.cmd.specs { + for _, spec := range p.cmd.specs() { // get the value defaultString, defaultValue, err := p.defaultVal(spec.dest) if err != nil { @@ -248,7 +260,7 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { // parseFieldsFromStructPointer ensures the destination structure is a pointer // to a struct. This function should be called when parsing commands or // subcommands as they can only be a struct pointer. -func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { +func (cmd *command) parseFieldsFromStructPointer(t reflect.Type, insideGroup bool) error { // commands can only be created from pointers to structs if t.Kind() != reflect.Ptr { return fmt.Errorf("subcommands must be pointers to structs but %s is a %s", @@ -260,7 +272,33 @@ func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { return fmt.Errorf("subcommands must be pointers to structs but %s is a pointer to %s", cmd.dest, t.Kind()) } + return cmd.parseStruct(t, insideGroup) +} +// parseFieldsFromStructOrStructPointer ensures the destination structure is +// either a pointer to a struct, or a struct. This function should be called +// when parsing option groups as they can only be a struct, or a pointer to one. +func (cmd *command) parseFieldsFromStructOrStructPointer(t reflect.Type, insideGroup bool) error { + // option groups can only be created from structs or pointers to structs + typeHint := "" + if t.Kind() == reflect.Ptr { + typeHint = "a pointer to " + t = t.Elem() + } + + if t.Kind() != reflect.Struct { + return fmt.Errorf("option groups must be structs or pointers to structs, but %s is %s%s", + cmd.dest, typeHint, t.Kind()) + } + + return cmd.parseStruct(t, insideGroup) +} + +// parseStruct populates the command instance based on the type and annotations +// of the target struct. As these command instances are used for either (sub) +// commands or option groups, please refer to the parseFieldsFromStructPointer +// or parseFieldsFromStructOrStructPointer respectively. +func (cmd *command) parseStruct(t reflect.Type, insideGroup bool) error { var errs []string walkFields(t, func(field reflect.StructField, t reflect.Type) bool { // check for the ignore switch in the tag @@ -342,19 +380,47 @@ func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { } cmd.subcommands = append(cmd.subcommands, &subCmd) + if insideGroup { + errs = append(errs, fmt.Sprintf("%s.%s: %s subcommands cannot be part of option groups", + t.Name(), field.Name, field.Type.String())) + return false + } + // decide on a name for the subcommand if subCmd.name == "" { subCmd.name = strings.ToLower(field.Name) } // parse the subcommand recursively - err := subCmd.parseFieldsFromStructPointer(field.Type) + err := subCmd.parseFieldsFromStructPointer(field.Type, false) if err != nil { errs = append(errs, err.Error()) return false } return true + case key == "group": + // parse the option group recursively + optGrp := command{ + name: value, + dest: subdest, + parent: cmd, + help: field.Tag.Get("help"), + } + cmd.groups = append(cmd.groups, &optGrp) + + // decide on a name for the group + if optGrp.name == "" { + optGrp.name = strings.Title(field.Name) + } + + err := optGrp.parseFieldsFromStructOrStructPointer(field.Type, true) + if err != nil { + errs = append(errs, err.Error()) + return false + } + + return false default: errs = append(errs, fmt.Sprintf("unrecognized tag '%s' on field %s", key, tag)) return false @@ -417,7 +483,7 @@ func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { } // add the spec to the list of specs - cmd.specs = append(cmd.specs, &spec) + cmd.options = append(cmd.options, &spec) // if this was an embedded field then we already returned true up above return false @@ -429,7 +495,7 @@ func (cmd *command) parseFieldsFromStructPointer(t reflect.Type) error { // check that we don't have both positionals and subcommands var hasPositional bool - for _, spec := range cmd.specs { + for _, spec := range cmd.options { if spec.positional { hasPositional = true } @@ -531,8 +597,7 @@ func (p *Parser) process(args []string) error { p.lastCmd = curCmd // make a copy of the specs because we will add to this list each time we expand a subcommand - specs := make([]*spec, len(curCmd.specs)) - copy(specs, curCmd.specs) + specs := curCmd.specs() // deal with environment vars if !p.config.IgnoreEnv { @@ -571,11 +636,11 @@ func (p *Parser) process(args []string) error { p.val(subcmd.dest) // add the new options to the set of allowed options - specs = append(specs, subcmd.specs...) + specs = append(specs, subcmd.specs()...) // capture environment vars for these new options if !p.config.IgnoreEnv { - err := p.captureEnvVars(subcmd.specs, wasPresent) + err := p.captureEnvVars(subcmd.specs(), wasPresent) if err != nil { return err } diff --git a/parse_test.go b/parse_test.go index 2fa3d92..fe3197a 100644 --- a/parse_test.go +++ b/parse_test.go @@ -288,6 +288,220 @@ func TestLongFlag(t *testing.T) { assert.Equal(t, "xyz", args.Foo) } +func TestGroupRequired(t *testing.T) { + var args struct { + Foo *struct { + Bar string `arg:"required"` + } `arg:"group"` + } + err := parse("", &args) + require.Error(t, err, "--bar is required") +} + +func TestNonPointerGroup(t *testing.T) { + var args struct { + Group struct { + Foo string + } `arg:"group"` + } + + _, err := NewParser(Config{IgnoreEnv: true}, &args) + require.NoError(t, err) +} + +func TestNonStructGroup(t *testing.T) { + var args struct { + NotGroup int `arg:"group"` + } + + _, err := NewParser(Config{IgnoreEnv: true}, &args) + require.Error(t, err, "option groups must be structs or pointers to structs, but args.NotGroup is int") +} + +func TestNonStructPtrGroup(t *testing.T) { + var args struct { + NotGroup *int `arg:"group"` + } + + _, err := NewParser(Config{IgnoreEnv: true}, &args) + require.Error(t, err, "option groups must be structs or pointers to structs, but args.NotGroup is a pointer to int") +} + +func TestNoSubcommandInGroup(t *testing.T) { + var args struct { + Group struct { + Sub *struct{} `arg:"subcommand"` + } `arg:"group"` + } + + _, err := NewParser(Config{IgnoreEnv: true}, &args) + require.Error(t, err, "subcommands cannot be part of option groups") +} + +func TestGroupParsingWithoutArguments(t *testing.T) { + type perm struct { + Anent string + } + + type opt struct { + Ional string + } + + type def struct { + Ault string `default:"permanent"` + } + + type args struct { + Foo string + PermanentGroup perm `arg:"group:Permanent"` + OptionalGroup *opt `arg:"group:Optional"` + OptionalWithDefault *def `arg:"group:With default"` + Input string `arg:"positional"` + } + + var opts args + err := parse("", &opts) + require.NoError(t, err) + assert.Equal(t, args{ + OptionalWithDefault: &def{ + Ault: "permanent", + }, + }, opts) +} + +func TestGroupParsingWithPositional(t *testing.T) { + type perm struct { + Anent string + } + + type opt struct { + Ional string + } + + type def struct { + Ault string `default:"permanent"` + } + + type args struct { + Foo string + PermanentGroup perm `arg:"group:Permanent"` + OptionalGroup *opt `arg:"group:Optional"` + OptionalWithDefault *def `arg:"group:With default"` + Input string `arg:"positional"` + } + + var opts args + err := parse("input", &opts) + require.NoError(t, err) + assert.Equal(t, args{ + OptionalWithDefault: &def{ + Ault: "permanent", + }, + Input: "input", + }, opts) +} + +func TestGroupParsingOfGroupPointer(t *testing.T) { + type perm struct { + Anent string + } + + type opt struct { + Ional string + } + + type def struct { + Ault string `default:"permanent"` + } + + type args struct { + Foo string + PermanentGroup perm `arg:"group:Permanent"` + OptionalGroup *opt `arg:"group:Optional"` + OptionalWithDefault *def `arg:"group:With default"` + Input string `arg:"positional"` + } + + var opts args + err := parse("--ional pointer", &opts) + require.NoError(t, err) + assert.Equal(t, args{ + OptionalGroup: &opt{ + Ional: "pointer", + }, + OptionalWithDefault: &def{ + Ault: "permanent", + }, + }, opts) +} + +func TestGroupParsingOfGroupStruct(t *testing.T) { + type perm struct { + Anent string + } + + type opt struct { + Ional string + } + + type def struct { + Ault string `default:"permanent"` + } + + type args struct { + Foo string + PermanentGroup perm `arg:"group:Permanent"` + OptionalGroup *opt `arg:"group:Optional"` + OptionalWithDefault *def `arg:"group:With default"` + Input string `arg:"positional"` + } + + var opts args + err := parse("--anent struct", &opts) + require.NoError(t, err) + assert.Equal(t, args{ + PermanentGroup: perm{ + Anent: "struct", + }, + OptionalWithDefault: &def{ + Ault: "permanent", + }, + }, opts) +} + +func TestGroupParsingWithMixedTypes(t *testing.T) { + type perm struct { + Anent string + } + + type opt struct { + Ional string + } + + type def struct { + Ault string `default:"permanent"` + } + + type args struct { + Foo string + PermanentGroup perm `arg:"group:Permanent"` + OptionalGroup *opt `arg:"group:Optional"` + OptionalWithDefault *def `arg:"group:With default"` + Input string `arg:"positional"` + } + + var opts args + err := parse("--foo bar -ional pointer --anent struct last", &opts) + require.NoError(t, err) + assert.Equal(t, args{ + Foo: "bar", + OptionalGroup: &opt{Ional: "pointer"}, + PermanentGroup: perm{Anent: "struct"}, + OptionalWithDefault: &def{Ault: "permanent"}, + Input: "last", + }, opts) +} + func TestSlice(t *testing.T) { var args struct { Strings []string @@ -705,6 +919,84 @@ func TestMustParse(t *testing.T) { assert.NotNil(t, parser) } +func TestNewParserWithEnv(t *testing.T) { + type simpleEnv struct { + Foo string `arg:"env"` + } + + // No env, no command line: value is empty + dest := &simpleEnv{} + _, err := parseWithEnv("", []string{}, dest) + require.NoError(t, err) + assert.Equal(t, &simpleEnv{Foo: ""}, dest) + + // With env, no command line: value takes environment + dest = &simpleEnv{} + _, err = parseWithEnv("", []string{"FOO=env"}, dest) + require.NoError(t, err) + assert.Equal(t, &simpleEnv{Foo: "env"}, dest) + + // No env, with command line: value takes argument + dest = &simpleEnv{} + _, err = parseWithEnv("--foo=cmd", []string{}, dest) + require.NoError(t, err) + assert.Equal(t, &simpleEnv{Foo: "cmd"}, dest) + + // With env, with command line: value takes argument + dest = &simpleEnv{} + _, err = parseWithEnv("--foo=cmd", []string{"FOO=env"}, dest) + require.NoError(t, err) + assert.Equal(t, &simpleEnv{Foo: "cmd"}, dest) +} + +func TestNewParserWithEnvAndDefault(t *testing.T) { + type envWithDefault struct { + Foo string `arg:"env" default:"def"` + } + + // No env, no command line: value takes default + dest := &envWithDefault{} + _, err := parseWithEnv("", []string{}, dest) + require.NoError(t, err) + assert.Equal(t, &envWithDefault{Foo: "def"}, dest) + + // With env, no command line: value takes environment + dest = &envWithDefault{} + _, err = parseWithEnv("", []string{"FOO=env"}, dest) + require.NoError(t, err) + assert.Equal(t, &envWithDefault{Foo: "env"}, dest) + + // No env, with command line: value takes argument + dest = &envWithDefault{} + _, err = parseWithEnv("--foo=cmd", []string{}, dest) + require.NoError(t, err) + assert.Equal(t, &envWithDefault{Foo: "cmd"}, dest) + + // With env, with command line: value takes argument + dest = &envWithDefault{} + _, err = parseWithEnv("--foo=cmd", []string{"FOO=env"}, dest) + require.NoError(t, err) + assert.Equal(t, &envWithDefault{Foo: "cmd"}, dest) +} + +func TestNewParserWithoutArgumentWithEnvAndDefault(t *testing.T) { + type noArgWithEnvAndWithDefault struct { + Foo string `arg:"--,env" default:"def"` + } + + // No env, no command line: value takes default + dest := &noArgWithEnvAndWithDefault{} + _, err := parseWithEnv("", []string{}, dest) + require.NoError(t, err) + assert.Equal(t, &noArgWithEnvAndWithDefault{Foo: "def"}, dest) + + // With env, no command line: value takes environment + dest = &noArgWithEnvAndWithDefault{} + _, err = parseWithEnv("", []string{"FOO=env"}, dest) + require.NoError(t, err) + assert.Equal(t, &noArgWithEnvAndWithDefault{Foo: "env"}, dest) +} + func TestEnvironmentVariable(t *testing.T) { var args struct { Foo string `arg:"env"` diff --git a/usage.go b/usage.go index 7a480c3..e8a1d49 100644 --- a/usage.go +++ b/usage.go @@ -70,7 +70,7 @@ func (p *Parser) WriteUsageForSubcommand(w io.Writer, subcommand ...string) erro // writeUsageForSubcommand writes usage information for the given subcommand func (p *Parser) writeUsageForSubcommand(w io.Writer, cmd *command) { var positionals, longOptions, shortOptions []*spec - for _, spec := range cmd.specs { + for _, spec := range cmd.specs() { switch { case spec.positional: positionals = append(positionals, spec) @@ -216,24 +216,18 @@ func (p *Parser) WriteHelpForSubcommand(w io.Writer, subcommand ...string) error // writeHelp writes the usage string for the given subcommand func (p *Parser) writeHelpForSubcommand(w io.Writer, cmd *command) { - var positionals, longOptions, shortOptions []*spec - for _, spec := range cmd.specs { - switch { - case spec.positional: - positionals = append(positionals, spec) - case spec.long != "": - longOptions = append(longOptions, spec) - case spec.short != "": - shortOptions = append(shortOptions, spec) - } - } - if p.description != "" { fmt.Fprintln(w, p.description) } p.writeUsageForSubcommand(w, cmd) // write the list of positionals + var positionals []*spec + for _, spec := range cmd.options { + if spec.positional { + positionals = append(positionals, spec) + } + } if len(positionals) > 0 { fmt.Fprint(w, "\nPositional arguments:\n") for _, spec := range positionals { @@ -242,26 +236,18 @@ func (p *Parser) writeHelpForSubcommand(w io.Writer, cmd *command) { } // write the list of options with the short-only ones first to match the usage string - if len(shortOptions)+len(longOptions) > 0 || cmd.parent == nil { - fmt.Fprint(w, "\nOptions:\n") - for _, spec := range shortOptions { - p.printOption(w, spec) - } - for _, spec := range longOptions { - p.printOption(w, spec) - } - } + p.writeHelpForArguments(w, cmd, "Options", "") // obtain a flattened list of options from all ancestors var globals []*spec ancestor := cmd.parent for ancestor != nil { - globals = append(globals, ancestor.specs...) + globals = append(globals, ancestor.specs()...) ancestor = ancestor.parent } // write the list of global options - if len(globals) > 0 { + if len(globals) > 0 || len(cmd.groups) > 0 { fmt.Fprint(w, "\nGlobal options:\n") for _, spec := range globals { p.printOption(w, spec) @@ -296,6 +282,45 @@ func (p *Parser) writeHelpForSubcommand(w io.Writer, cmd *command) { } } +// writeHelpForArguments writes the list of short, long, and environment-only +// options in order. +func (p *Parser) writeHelpForArguments(w io.Writer, cmd *command, header, help string) { + var positionals, longOptions, shortOptions []*spec + for _, spec := range cmd.options { + switch { + case spec.positional: + positionals = append(positionals, spec) + case spec.long != "": + longOptions = append(longOptions, spec) + case spec.short != "": + shortOptions = append(shortOptions, spec) + } + } + + if cmd.parent != nil && len(shortOptions)+len(longOptions) == 0 { + return + } + + // write the list of options with the short-only ones first to match the usage string + fmt.Fprintf(w, "\n%v:\n", header) + if help != "" { + fmt.Fprintf(w, "\n%v\n\n", help) + } + for _, spec := range shortOptions { + p.printOption(w, spec) + } + for _, spec := range longOptions { + p.printOption(w, spec) + } + + // write the list of argument groups + if len(cmd.groups) > 0 { + for _, grpCmd := range cmd.groups { + p.writeHelpForArguments(w, grpCmd, fmt.Sprintf("%s options", grpCmd.name), grpCmd.help) + } + } +} + func (p *Parser) printOption(w io.Writer, spec *spec) { ways := make([]string, 0, 2) if spec.long != "" { @@ -304,6 +329,9 @@ func (p *Parser) printOption(w io.Writer, spec *spec) { if spec.short != "" { ways = append(ways, synopsis(spec, "-"+spec.short)) } + if spec.env != "" && len(ways) == 0 { + ways = append(ways, "(environment only)") + } if len(ways) > 0 { printTwoCols(w, strings.Join(ways, ", "), spec.help, spec.defaultString, spec.env) } diff --git a/usage_test.go b/usage_test.go index be5894a..0350521 100644 --- a/usage_test.go +++ b/usage_test.go @@ -50,7 +50,7 @@ Options: --optimize OPTIMIZE, -O OPTIMIZE optimization level --ids IDS Ids - --values VALUES Values + --values VALUES Values [default: [3.14 42 256]] --workers WORKERS, -w WORKERS number of workers to start [default: 10, env: WORKERS] --testenv TESTENV, -a TESTENV [env: TEST_ENV] @@ -74,6 +74,7 @@ Options: } args.Name = "Foo Bar" args.Value = 42 + args.Values = []float64{3.14, 42, 256} args.File = &NameDotName{"scratch", "txt"} p, err := NewParser(Config{Program: "example"}, &args) require.NoError(t, err) @@ -489,6 +490,200 @@ func TestNonexistentSubcommand(t *testing.T) { assert.Error(t, err) } +func TestUsageWithOptionGroup(t *testing.T) { + expectedUsage := "Usage: example [--verbose] [--insecure] [--host HOST] [--port PORT] [--user USER] OUTPUT" + + expectedHelp := ` +Usage: example [--verbose] [--insecure] [--host HOST] [--port PORT] [--user USER] OUTPUT + +Positional arguments: + OUTPUT + +Options: + --verbose, -v verbosity level + +Database options: + +This block represents related arguments. + + --insecure, -i disable tls + --host HOST hostname to connect to [default: localhost, env: DB_HOST] + --port PORT port to connect to [default: 3306, env: DB_PORT] + --user USER username to connect as [env: DB_USERNAME] + +Global options: + --help, -h display this help and exit +` + + type database struct { + Insecure bool `arg:"-i,--insecure" help:"disable tls"` + Host string `arg:"--host,env:DB_HOST" help:"hostname to connect to" default:"localhost"` + Port string `arg:"--port,env:DB_PORT" help:"port to connect to" default:"3306"` + User string `arg:"--user,env:DB_USERNAME" help:"username to connect as"` + Password string `arg:"--,env:DB_PASSWORD" help:"password to connect with"` + } + + var args struct { + Verbose bool `arg:"-v" help:"verbosity level"` + Database *database `arg:"group" help:"This block represents related arguments."` + Output string `arg:"positional,required"` + } + + os.Args[0] = "example" + p, err := NewParser(Config{}, &args) + require.NoError(t, err) + + _ = p.Parse([]string{}) + + var help bytes.Buffer + p.WriteHelp(&help) + assert.Equal(t, expectedHelp[1:], help.String()) + + var usage bytes.Buffer + p.WriteUsage(&usage) + assert.Equal(t, expectedUsage, strings.TrimSpace(usage.String())) +} + +func TestUsageWithoutSubcommandAndOptionGroup(t *testing.T) { + expectedUsage := "Usage: example [-s] [--global] []" + + expectedHelp := ` +Usage: example [-s] [--global] [] + +Options: + --global, -g global option + +Global group options: + +This block represents related arguments. + + -s global something + +Global options: + --help, -h display this help and exit + +Commands: + foo Command A + bar Command B +` + + var args struct { + Global bool `arg:"-g" help:"global option"` + GlobalGroup *struct { + Something bool `arg:"-s,--" help:"global something"` + } `arg:"group:Global group" help:"This block represents related arguments."` + CommandA *struct { + OptionA bool `arg:"-a,--" help:"option for sub A"` + GroupA *struct { + GroupA bool `arg:"--group-a" help:"group belonging to cmd A"` + } `arg:"group:Group A" help:"This block belongs to command A."` + } `arg:"subcommand:foo" help:"Command A"` + CommandB *struct { + OptionB bool `arg:"-b,--" help:"option for sub B"` + GroupB *struct { + GroupB bool `arg:"--group-b" help:"group belonging to cmd B"` + NestedGroup *struct { + NestedGroup bool `arg:"--nested-group" help:"nested group belonging to group B of cmd B"` + } `arg:"group:Nested Group" help:"This block belongs to group B of command B."` + } `arg:"group:Group B" help:"This block belongs to command B."` + } `arg:"subcommand:bar" help:"Command B"` + } + + os.Args[0] = "example" + p, err := NewParser(Config{}, &args) + require.NoError(t, err) + + _ = p.Parse([]string{}) + + var help bytes.Buffer + p.WriteHelp(&help) + assert.Equal(t, expectedHelp[1:], help.String()) + + var help2 bytes.Buffer + p.WriteHelpForSubcommand(&help2) + assert.Equal(t, expectedHelp[1:], help2.String()) + + var usage bytes.Buffer + p.WriteUsage(&usage) + assert.Equal(t, expectedUsage, strings.TrimSpace(usage.String())) + + var usage2 bytes.Buffer + p.WriteUsageForSubcommand(&usage2) + assert.Equal(t, expectedUsage, strings.TrimSpace(usage2.String())) +} + +func TestUsageWithSubcommandAndOptionGroup(t *testing.T) { + + expectedUsage := "Usage: example bar [-b] [--group-b] [--nested-group]" + expectedHelp := ` +Usage: example bar [-b] [--group-b] [--nested-group] + +Options: + -b option for sub B + +Group B options: + +This block belongs to command B. + + --group-b group belonging to cmd B + +Nested Group options: + +This block belongs to group B of command B. + + --nested-group nested group belonging to group B of cmd B + +Global options: + --global, -g global option + -s global something + --help, -h display this help and exit +` + + var args struct { + Global bool `arg:"-g" help:"global option"` + GlobalGroup *struct { + Something bool `arg:"-s,--" help:"global something"` + } `arg:"group:Global group" help:"This block represents related arguments."` + CommandA *struct { + OptionA bool `arg:"-a,--" help:"option for sub A"` + GroupA *struct { + GroupA bool `arg:"--group-a" help:"group belonging to cmd A"` + } `arg:"group:Group A" help:"This block belongs to command A."` + } `arg:"subcommand:foo" help:"Command A"` + CommandB *struct { + OptionB bool `arg:"-b,--" help:"option for sub B"` + GroupB *struct { + GroupB bool `arg:"--group-b" help:"group belonging to cmd B"` + NestedGroup *struct { + NestedGroup bool `arg:"--nested-group" help:"nested group belonging to group B of cmd B"` + } `arg:"group:Nested Group" help:"This block belongs to group B of command B."` + } `arg:"group:Group B" help:"This block belongs to command B."` + } `arg:"subcommand:bar" help:"Command B"` + } + + os.Args[0] = "example" + p, err := NewParser(Config{}, &args) + require.NoError(t, err) + + _ = p.Parse([]string{"bar"}) + + var help bytes.Buffer + p.WriteHelp(&help) + assert.Equal(t, expectedHelp[1:], help.String()) + + var help2 bytes.Buffer + p.WriteHelpForSubcommand(&help2, "bar") + assert.Equal(t, expectedHelp[1:], help2.String()) + + var usage bytes.Buffer + p.WriteUsage(&usage) + assert.Equal(t, expectedUsage, strings.TrimSpace(usage.String())) + + var usage2 bytes.Buffer + p.WriteUsageForSubcommand(&usage2, "bar") + assert.Equal(t, expectedUsage, strings.TrimSpace(usage2.String())) +} + func TestUsageWithoutLongNames(t *testing.T) { expectedUsage := "Usage: example [-a PLACEHOLDER] -b SHORTONLY2" @@ -505,7 +700,7 @@ Options: ShortOnly2 string `arg:"-b,--,required" help:"some help2"` } p, err := NewParser(Config{Program: "example"}, &args) - require.NoError(t, err) + assert.NoError(t, err) var help bytes.Buffer p.WriteHelp(&help) From ac7590cca756091072bd422cc8380a0eadc4f18f Mon Sep 17 00:00:00 2001 From: Sebastiaan Pasterkamp <26205277+SebastiaanPasterkamp@users.noreply.github.com> Date: Sat, 10 Dec 2022 15:29:56 +0100 Subject: [PATCH 5/5] feat(help): Include environment-only variables The environment only variables don't accept command line options, but should still be listed in the help message. Instead of listing the command line flags the placeholder text '(environment only)' is displayed. --- README.md | 1 + example_test.go | 1 + usage.go | 9 +++++++-- usage_test.go | 3 +++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index a891a61..60d6121 100644 --- a/README.md +++ b/README.md @@ -634,6 +634,7 @@ Change the default registry to push to. --host HOST URL of the repository [default: docker.io] --user USER username to connect as [env: REPO_USERNAME] + (environment only) password to connect with [env: REPO_PASSWORD] Global options: --quiet, -q Quiet diff --git a/example_test.go b/example_test.go index 2bd74a2..0e21589 100644 --- a/example_test.go +++ b/example_test.go @@ -296,6 +296,7 @@ func Example_helpTextWithGroups() { // // --host HOST URL of the repository [default: docker.io] // --user USER username to connect as [env: REPO_USERNAME] + // (environment only) password to connect with [env: REPO_PASSWORD] // // Global options: // --quiet, -q Quiet diff --git a/usage.go b/usage.go index e8a1d49..b18e6f2 100644 --- a/usage.go +++ b/usage.go @@ -285,7 +285,7 @@ func (p *Parser) writeHelpForSubcommand(w io.Writer, cmd *command) { // writeHelpForArguments writes the list of short, long, and environment-only // options in order. func (p *Parser) writeHelpForArguments(w io.Writer, cmd *command, header, help string) { - var positionals, longOptions, shortOptions []*spec + var positionals, longOptions, shortOptions, envOnly []*spec for _, spec := range cmd.options { switch { case spec.positional: @@ -294,10 +294,12 @@ func (p *Parser) writeHelpForArguments(w io.Writer, cmd *command, header, help s longOptions = append(longOptions, spec) case spec.short != "": shortOptions = append(shortOptions, spec) + case spec.env != "": + envOnly = append(envOnly, spec) } } - if cmd.parent != nil && len(shortOptions)+len(longOptions) == 0 { + if cmd.parent != nil && len(shortOptions)+len(longOptions)+len(envOnly) == 0 { return } @@ -312,6 +314,9 @@ func (p *Parser) writeHelpForArguments(w io.Writer, cmd *command, header, help s for _, spec := range longOptions { p.printOption(w, spec) } + for _, spec := range envOnly { + p.printOption(w, spec) + } // write the list of argument groups if len(cmd.groups) > 0 { diff --git a/usage_test.go b/usage_test.go index 0350521..7add765 100644 --- a/usage_test.go +++ b/usage_test.go @@ -510,6 +510,7 @@ This block represents related arguments. --host HOST hostname to connect to [default: localhost, env: DB_HOST] --port PORT port to connect to [default: 3306, env: DB_PORT] --user USER username to connect as [env: DB_USERNAME] + (environment only) password to connect with [env: DB_PASSWORD] Global options: --help, -h display this help and exit @@ -746,6 +747,8 @@ Usage: example [-s SHORT] Options: -s SHORT [env: SHORT] + (environment only) [env: ENVONLY] + (environment only) [env: CUSTOM] --help, -h display this help and exit ` var args struct {