From 0c952979908ba4db23086ffdbdfa3bca01f174e0 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Tue, 8 Oct 2019 16:39:00 -0700 Subject: [PATCH 1/8] add support for default values in struct tags --- go.mod | 2 ++ parse.go | 39 ++++++++++++++++++++++++----- parse_test.go | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index c4c4879..14c6119 100644 --- a/go.mod +++ b/go.mod @@ -4,3 +4,5 @@ require ( github.com/alexflint/go-scalar v1.0.0 github.com/stretchr/testify v1.2.2 ) + +go 1.13 diff --git a/parse.go b/parse.go index a29258a..d234ed2 100644 --- a/parse.go +++ b/parse.go @@ -54,6 +54,7 @@ type spec struct { help string env string boolean bool + defaultVal string // default value for this option, only if provided as a struct tag } // command represents a named subcommand, or the top-level command @@ -250,6 +251,11 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { spec.help = help } + defaultVal, hasDefault := field.Tag.Lookup("default") + if hasDefault { + spec.defaultVal = defaultVal + } + // Look at the tag var isSubcommand bool // tracks whether this field is a subcommand if tag != "" { @@ -274,6 +280,11 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { } spec.short = key[1:] case key == "required": + if hasDefault { + errs = append(errs, fmt.Sprintf("%s.%s: 'required' cannot be used when a default value is specified", + t.Name(), field.Name)) + return false + } spec.required = true case key == "positional": spec.positional = true @@ -328,6 +339,11 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { t.Name(), field.Name, field.Type.String())) return false } + if spec.multiple && hasDefault { + errs = append(errs, fmt.Sprintf("%s.%s: default values are not supported for slice fields", + t.Name(), field.Name)) + return false + } } // if this was an embedded field then we already returned true up above @@ -570,15 +586,26 @@ func (p *Parser) process(args []string) error { return fmt.Errorf("too many positional arguments at '%s'", positionals[0]) } - // finally check that all the required args were provided + // fill in defaults and check that all the required args were provided for _, spec := range specs { - if spec.required && !wasPresent[spec] { - name := spec.long - if !spec.positional { - name = "--" + spec.long - } + if wasPresent[spec] { + continue + } + + name := spec.long + if !spec.positional { + name = "--" + spec.long + } + + if spec.required { return fmt.Errorf("%s is required", name) } + if spec.defaultVal != "" { + err := scalar.ParseValue(p.val(spec.dest), spec.defaultVal) + if err != nil { + return fmt.Errorf("error processing default value for %s: %v", name, err) + } + } } return nil diff --git a/parse_test.go b/parse_test.go index 5909472..9cd8bce 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1057,3 +1057,71 @@ func TestMultipleTerminates(t *testing.T) { assert.Equal(t, []string{"a", "b"}, args.X) assert.Equal(t, "c", args.Y) } + +func TestDefaultOptionValues(t *testing.T) { + var args struct { + A int `default:"123"` + B *int `default:"123"` + C string `default:"abc"` + D *string `default:"abc"` + E float64 `default:"1.23"` + F *float64 `default:"1.23"` + G bool `default:"true"` + H *bool `default:"true"` + } + + err := parse("--c=xyz --e=4.56", &args) + require.NoError(t, err) + + assert.Equal(t, 123, args.A) + assert.Equal(t, 123, *args.B) + assert.Equal(t, "xyz", args.C) + assert.Equal(t, "abc", *args.D) + assert.Equal(t, 4.56, args.E) + assert.Equal(t, 1.23, *args.F) + assert.True(t, args.G) + assert.True(t, args.G) +} + +func TestDefaultPositionalValues(t *testing.T) { + var args struct { + A int `arg:"positional" default:"123"` + B *int `arg:"positional" default:"123"` + C string `arg:"positional" default:"abc"` + D *string `arg:"positional" default:"abc"` + E float64 `arg:"positional" default:"1.23"` + F *float64 `arg:"positional" default:"1.23"` + G bool `arg:"positional" default:"true"` + H *bool `arg:"positional" default:"true"` + } + + err := parse("456 789", &args) + require.NoError(t, err) + + assert.Equal(t, 456, args.A) + assert.Equal(t, 789, *args.B) + assert.Equal(t, "abc", args.C) + assert.Equal(t, "abc", *args.D) + assert.Equal(t, 1.23, args.E) + assert.Equal(t, 1.23, *args.F) + assert.True(t, args.G) + assert.True(t, args.G) +} + +func TestDefaultValuesNotAllowedWithRequired(t *testing.T) { + var args struct { + A int `arg:"required" default:"123"` // required not allowed with default! + } + + err := parse("", &args) + assert.EqualError(t, err, ".A: 'required' cannot be used when a default value is specified") +} + +func TestDefaultValuesNotAllowedWithSlice(t *testing.T) { + var args struct { + A []int `default:"123"` // required not allowed with default! + } + + err := parse("", &args) + assert.EqualError(t, err, ".A: default values are not supported for slice fields") +} From 5d3ebcceeee6ce36f5f7244f7cd9600d9823748e Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Tue, 8 Oct 2019 16:47:31 -0700 Subject: [PATCH 2/8] undo changes to go.mod --- go.mod | 2 -- 1 file changed, 2 deletions(-) diff --git a/go.mod b/go.mod index 14c6119..c4c4879 100644 --- a/go.mod +++ b/go.mod @@ -4,5 +4,3 @@ require ( github.com/alexflint/go-scalar v1.0.0 github.com/stretchr/testify v1.2.2 ) - -go 1.13 From cc768447a7257b5957349efe0f7ecbaaa95f34f8 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 19 Oct 2019 23:23:32 -0700 Subject: [PATCH 3/8] store default values during NewParser --- parse.go | 30 ++++++++++++++++++++++++++++++ parse_test.go | 9 +++++++++ usage.go | 36 ++++++------------------------------ usage_test.go | 33 ++++++++++++++++++++++----------- 4 files changed, 67 insertions(+), 41 deletions(-) diff --git a/parse.go b/parse.go index d234ed2..e630e4b 100644 --- a/parse.go +++ b/parse.go @@ -1,6 +1,7 @@ package arg import ( + "encoding" "encoding/csv" "errors" "fmt" @@ -193,6 +194,22 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { if err != nil { return nil, err } + + // add nonzero field values as defaults + for _, spec := range cmd.specs { + if v := p.val(spec.dest); v.IsValid() && !isZero(v) { + if defaultVal, ok := v.Interface().(encoding.TextMarshaler); ok { + str, err := defaultVal.MarshalText() + if err != nil { + return nil, fmt.Errorf("%v: error marshaling default value to string: %w", spec.dest, err) + } + spec.defaultVal = string(str) + } else { + spec.defaultVal = fmt.Sprintf("%v", v) + } + } + } + p.cmd.specs = append(p.cmd.specs, cmd.specs...) p.cmd.subcommands = append(p.cmd.subcommands, cmd.subcommands...) @@ -706,3 +723,16 @@ func findSubcommand(cmds []*command, name string) *command { } return nil } + +// isZero returns true if v contains the zero value for its type +func isZero(v reflect.Value) bool { + t := v.Type() + if t.Kind() == reflect.Slice { + return v.IsNil() + } + if !t.Comparable() { + return false + } + z := reflect.Zero(t) + return v.Interface() == z.Interface() +} diff --git a/parse_test.go b/parse_test.go index 9cd8bce..47e9ccd 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1083,6 +1083,15 @@ func TestDefaultOptionValues(t *testing.T) { assert.True(t, args.G) } +func TestDefaultUnparseable(t *testing.T) { + var args struct { + A int `default:"x"` + } + + err := parse("", &args) + assert.EqualError(t, err, `error processing default value for --a: strconv.ParseInt: parsing "x": invalid syntax`) +} + func TestDefaultPositionalValues(t *testing.T) { var args struct { A int `arg:"positional" default:"123"` diff --git a/usage.go b/usage.go index 33b6184..43db703 100644 --- a/usage.go +++ b/usage.go @@ -1,11 +1,9 @@ package arg import ( - "encoding" "fmt" "io" "os" - "reflect" "strings" ) @@ -94,7 +92,7 @@ func (p *Parser) writeUsageForCommand(w io.Writer, cmd *command) { fmt.Fprint(w, "\n") } -func printTwoCols(w io.Writer, left, help string, defaultVal *string) { +func printTwoCols(w io.Writer, left, help string, defaultVal string) { lhs := " " + left fmt.Fprint(w, lhs) if help != "" { @@ -105,8 +103,8 @@ func printTwoCols(w io.Writer, left, help string, defaultVal *string) { } fmt.Fprint(w, help) } - if defaultVal != nil { - fmt.Fprintf(w, " [default: %s]", *defaultVal) + if defaultVal != "" { + fmt.Fprintf(w, " [default: %s]", defaultVal) } fmt.Fprint(w, "\n") } @@ -136,7 +134,7 @@ func (p *Parser) writeHelpForCommand(w io.Writer, cmd *command) { if len(positionals) > 0 { fmt.Fprint(w, "\nPositional arguments:\n") for _, spec := range positionals { - printTwoCols(w, strings.ToUpper(spec.long), spec.help, nil) + printTwoCols(w, strings.ToUpper(spec.long), spec.help, "") } } @@ -165,7 +163,7 @@ func (p *Parser) writeHelpForCommand(w io.Writer, cmd *command) { if len(cmd.subcommands) > 0 { fmt.Fprint(w, "\nCommands:\n") for _, subcmd := range cmd.subcommands { - printTwoCols(w, subcmd.name, subcmd.help, nil) + printTwoCols(w, subcmd.name, subcmd.help, "") } } } @@ -175,29 +173,7 @@ func (p *Parser) printOption(w io.Writer, spec *spec) { if spec.short != "" { left += ", " + synopsis(spec, "-"+spec.short) } - - // If spec.dest is not the zero value then a default value has been added. - var v reflect.Value - if len(spec.dest.fields) > 0 { - v = p.val(spec.dest) - } - - var defaultVal *string - if v.IsValid() { - z := reflect.Zero(v.Type()) - if (v.Type().Comparable() && z.Type().Comparable() && v.Interface() != z.Interface()) || v.Kind() == reflect.Slice && !v.IsNil() { - if scalar, ok := v.Interface().(encoding.TextMarshaler); ok { - if value, err := scalar.MarshalText(); err != nil { - defaultVal = ptrTo(fmt.Sprintf("error: %v", err)) - } else { - defaultVal = ptrTo(fmt.Sprintf("%v", string(value))) - } - } else { - defaultVal = ptrTo(fmt.Sprintf("%v", v)) - } - } - } - printTwoCols(w, left, spec.help, defaultVal) + printTwoCols(w, left, spec.help, spec.defaultVal) } func synopsis(spec *spec, form string) string { diff --git a/usage_test.go b/usage_test.go index fc0b8c5..d9d33f0 100644 --- a/usage_test.go +++ b/usage_test.go @@ -96,26 +96,37 @@ func (n *MyEnum) MarshalText() ([]byte, error) { return nil, errors.New("There was a problem") } -func TestUsageError(t *testing.T) { - expectedHelp := `Usage: example [--name NAME] +func TestUsageWithDefaults(t *testing.T) { + expectedHelp := `Usage: example [--label LABEL] [--content CONTENT] Options: - --name NAME [default: error: There was a problem] + --label LABEL [default: cat] + --content CONTENT [default: dog] --help, -h display this help and exit ` + var args struct { + Label string + Content string `default:"dog"` + } + args.Label = "cat" + p, err := NewParser(Config{"example"}, &args) + require.NoError(t, err) + + args.Label = "should_ignore_this" + + var help bytes.Buffer + p.WriteHelp(&help) + assert.Equal(t, expectedHelp, help.String()) +} + +func TestUsageCannotMarshalToString(t *testing.T) { var args struct { Name *MyEnum } v := MyEnum(42) args.Name = &v - p, err := NewParser(Config{"example"}, &args) - - // NB: some might might expect there to be an error here - require.NoError(t, err) - - var help bytes.Buffer - p.WriteHelp(&help) - assert.Equal(t, expectedHelp, help.String()) + _, err := NewParser(Config{"example"}, &args) + assert.EqualError(t, err, `args.Name: error marshaling default value to string: There was a problem`) } func TestUsageLongPositionalWithHelp_legacyForm(t *testing.T) { From 84e7a764db3c0b7970633340bc988630a8dd5dc2 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 19 Oct 2019 23:30:33 -0700 Subject: [PATCH 4/8] minor cleanups --- parse.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/parse.go b/parse.go index e630e4b..91bb35e 100644 --- a/parse.go +++ b/parse.go @@ -55,7 +55,7 @@ type spec struct { help string env string boolean bool - defaultVal string // default value for this option, only if provided as a struct tag + defaultVal string // default value for this option } // command represents a named subcommand, or the top-level command @@ -733,6 +733,5 @@ func isZero(v reflect.Value) bool { if !t.Comparable() { return false } - z := reflect.Zero(t) - return v.Interface() == z.Interface() + return v.Interface() == reflect.Zero(t).Interface() } From 45d0915afc6ce6efd1e625df07e3c7f4b72b8cc3 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Mon, 21 Oct 2019 11:42:03 -0700 Subject: [PATCH 5/8] Remove %w for compatibility with go<1.13 --- parse.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse.go b/parse.go index 91bb35e..bc156df 100644 --- a/parse.go +++ b/parse.go @@ -201,7 +201,7 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { if defaultVal, ok := v.Interface().(encoding.TextMarshaler); ok { str, err := defaultVal.MarshalText() if err != nil { - return nil, fmt.Errorf("%v: error marshaling default value to string: %w", spec.dest, err) + return nil, fmt.Errorf("%v: error marshaling default value to string: %v", spec.dest, err) } spec.defaultVal = string(str) } else { From 809e9060d082454ac2ece1960fa2c584ba01816d Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Mon, 21 Oct 2019 23:06:31 -0700 Subject: [PATCH 6/8] stop testing with tip on travis --- .travis.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index d2a00fd..87ef507 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,7 @@ language: go go: - - "1.10" - "1.12" - - tip -env: - - GO111MODULE=on # will only be used in go 1.11 + - "1.13" before_install: - go get github.com/axw/gocov/gocov - go get github.com/mattn/goveralls From 7ac060af1863205e7dfcc103b90711767e45e2a8 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Mon, 21 Oct 2019 23:13:41 -0700 Subject: [PATCH 7/8] update documentation to new way of specifying defaults --- README.md | 6 ++---- example_test.go | 5 ++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index c516c51..0ad03bc 100644 --- a/README.md +++ b/README.md @@ -142,10 +142,9 @@ Options: ```go var args struct { - Foo string + Foo string `default:"abc"` Bar bool } -args.Foo = "default value" arg.MustParse(&args) ``` @@ -307,9 +306,8 @@ func (n *NameDotName) MarshalText() ([]byte, error) { func main() { var args struct { - Name NameDotName + Name NameDotName `default:"file.txt"` } - args.Name = NameDotName{"file", "txt"} // set default value arg.MustParse(&args) fmt.Printf("%#v\n", args.Name) } diff --git a/example_test.go b/example_test.go index 2188253..f71fbeb 100644 --- a/example_test.go +++ b/example_test.go @@ -30,12 +30,11 @@ func Example_defaultValues() { os.Args = split("./example") var args struct { - Foo string + Foo string `default:"abc"` } - args.Foo = "default value" MustParse(&args) fmt.Println(args.Foo) - // output: default value + // output: abc } // This example demonstrates arguments that are required From e0fc08f7ad001371541770efcc43cf840288fee8 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Mon, 21 Oct 2019 23:37:12 -0700 Subject: [PATCH 8/8] add docs about old way of specifying defaults --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 0ad03bc..1f02559 100644 --- a/README.md +++ b/README.md @@ -148,6 +148,17 @@ var args struct { arg.MustParse(&args) ``` +### Default values (before v1.2) + +```go +var args struct { + Foo string + Bar bool +} +arg.Foo = "abc" +arg.MustParse(&args) +``` + ### Arguments with multiple values ```go var args struct {