From 23b2b67fe299b63a072a3541f34d57757d0b8df0 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Thu, 9 Jun 2022 11:21:29 -0400 Subject: [PATCH 1/9] fix issue #184 --- go.mod | 8 ++++++- parse.go | 43 +++++++++++++++++++++++++-------- parse_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ reflect.go | 19 ++++++++++++--- usage_test.go | 3 +-- 5 files changed, 123 insertions(+), 16 deletions(-) diff --git a/go.mod b/go.mod index 67ac880..0823012 100644 --- a/go.mod +++ b/go.mod @@ -5,4 +5,10 @@ require ( github.com/stretchr/testify v1.7.0 ) -go 1.13 +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect +) + +go 1.18 diff --git a/parse.go b/parse.go index 7588dfb..28ed014 100644 --- a/parse.go +++ b/parse.go @@ -208,18 +208,41 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { return nil, err } - // add nonzero field values as defaults + // for backwards compatibility, 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: %v", spec.dest, err) - } - spec.defaultVal = string(str) - } else { - spec.defaultVal = fmt.Sprintf("%v", v) + // do not read default when UnmarshalText is implemented but not MarshalText + if isTextUnmarshaler(spec.field.Type) && !isTextMarshaler(spec.field.Type) { + continue + } + + // do not process types that require multiple values + cardinality, _ := cardinalityOf(spec.field.Type) + if cardinality != one { + continue + } + + // get the value + v := p.val(spec.dest) + if !v.IsValid() { + continue + } + + // if MarshalText is implemented then use that + if m, ok := v.Interface().(encoding.TextMarshaler); ok { + if v.IsNil() { + continue } + s, err := m.MarshalText() + if err != nil { + return nil, fmt.Errorf("%v: error marshaling default value to string: %v", spec.dest, err) + } + spec.defaultVal = string(s) + continue + } + + // finally, use the value as a default if it is non-zero + if !isZero(v) { + spec.defaultVal = fmt.Sprintf("%v", v) } } diff --git a/parse_test.go b/parse_test.go index 2d0ef7a..0d58598 100644 --- a/parse_test.go +++ b/parse_test.go @@ -2,6 +2,7 @@ package arg import ( "bytes" + "encoding/json" "fmt" "net" "net/mail" @@ -1456,3 +1457,68 @@ func TestMustParsePrintsVersion(t *testing.T) { assert.Equal(t, 0, *exitCode) assert.Equal(t, "example 3.2.1\n", b.String()) } + +type jsonMap struct { + val map[string]string +} + +func (v *jsonMap) UnmarshalText(data []byte) error { + return json.Unmarshal(data, &v.val) +} + +func TestTextUnmarshallerEmpty(t *testing.T) { + // based on https://github.com/alexflint/go-arg/issues/184 + var args struct { + Config jsonMap `arg:"--config"` + } + + err := parse("", &args) + require.NoError(t, err) + assert.Empty(t, args.Config) +} + +func TestTextUnmarshallerEmptyPointer(t *testing.T) { + // a slight variant on https://github.com/alexflint/go-arg/issues/184 + var args struct { + Config *jsonMap `arg:"--config"` + } + + err := parse("", &args) + require.NoError(t, err) + assert.Nil(t, args.Config) +} + +// similar to the above but also implements MarshalText +type jsonMap2[T any] struct { + val T +} + +func (v *jsonMap2[T]) MarshalText(data []byte) error { + return json.Unmarshal(data, &v.val) +} + +func (v *jsonMap2[T]) UnmarshalText(data []byte) error { + return json.Unmarshal(data, &v.val) +} + +func TestTextMarshallerUnmarshallerEmpty(t *testing.T) { + // based on https://github.com/alexflint/go-arg/issues/184 + var args struct { + Config jsonMap2[map[string]string] `arg:"--config"` + } + + err := parse("", &args) + require.NoError(t, err) + assert.Empty(t, args.Config) +} + +func TestTextMarshallerUnmarshallerEmptyPointer(t *testing.T) { + // a slight variant on https://github.com/alexflint/go-arg/issues/184 + var args struct { + Config *jsonMap2[map[string]string] `arg:"--config"` + } + + err := parse("", &args) + require.NoError(t, err) + assert.Nil(t, args.Config) +} diff --git a/reflect.go b/reflect.go index cd80be7..b87db2a 100644 --- a/reflect.go +++ b/reflect.go @@ -10,7 +10,10 @@ import ( scalar "github.com/alexflint/go-scalar" ) -var textUnmarshalerType = reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem() +var ( + textMarshalerType = reflect.TypeOf([]encoding.TextMarshaler{}).Elem() + textUnmarshalerType = reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem() +) // cardinality tracks how many tokens are expected for a given spec // - zero is a boolean, which does to expect any value @@ -74,10 +77,10 @@ func cardinalityOf(t reflect.Type) (cardinality, error) { } } -// isBoolean returns true if the type can be parsed from a single string +// isBoolean returns true if the type is a boolean or a pointer to a boolean func isBoolean(t reflect.Type) bool { switch { - case t.Implements(textUnmarshalerType): + case isTextUnmarshaler(t): return false case t.Kind() == reflect.Bool: return true @@ -88,6 +91,16 @@ func isBoolean(t reflect.Type) bool { } } +// isTextMarshaler returns true if the type or its pointer implements encoding.TextMarshaler +func isTextMarshaler(t reflect.Type) bool { + return t.Implements(textMarshalerType) || reflect.PtrTo(t).Implements(textMarshalerType) +} + +// isTextUnmarshaler returns true if the type or its pointer implements encoding.TextUnmarshaler +func isTextUnmarshaler(t reflect.Type) bool { + return t.Implements(textUnmarshalerType) || reflect.PtrTo(t).Implements(textUnmarshalerType) +} + // isExported returns true if the struct field name is exported func isExported(field string) bool { r, _ := utf8.DecodeRuneInString(field) // returns RuneError for empty string or invalid UTF8 diff --git a/usage_test.go b/usage_test.go index 1744536..0a7ddd8 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 [default: [3.14 42 256]] + --values VALUES Values --workers WORKERS, -w WORKERS number of workers to start [default: 10, env: WORKERS] --testenv TESTENV, -a TESTENV [env: TEST_ENV] @@ -74,7 +74,6 @@ 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) From 197e226c771464f7c0b66cc2a2bbfb646010dc52 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 12:28:06 -0400 Subject: [PATCH 2/9] drop unnecessary use of templates in this test --- parse_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/parse_test.go b/parse_test.go index 0d58598..4dcb806 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1458,18 +1458,18 @@ func TestMustParsePrintsVersion(t *testing.T) { assert.Equal(t, "example 3.2.1\n", b.String()) } -type jsonMap struct { +type mapWithUnmarshalText struct { val map[string]string } -func (v *jsonMap) UnmarshalText(data []byte) error { +func (v *mapWithUnmarshalText) UnmarshalText(data []byte) error { return json.Unmarshal(data, &v.val) } -func TestTextUnmarshallerEmpty(t *testing.T) { +func TestTextUnmarshalerEmpty(t *testing.T) { // based on https://github.com/alexflint/go-arg/issues/184 var args struct { - Config jsonMap `arg:"--config"` + Config mapWithUnmarshalText `arg:"--config"` } err := parse("", &args) @@ -1477,10 +1477,10 @@ func TestTextUnmarshallerEmpty(t *testing.T) { assert.Empty(t, args.Config) } -func TestTextUnmarshallerEmptyPointer(t *testing.T) { +func TestTextUnmarshalerEmptyPointer(t *testing.T) { // a slight variant on https://github.com/alexflint/go-arg/issues/184 var args struct { - Config *jsonMap `arg:"--config"` + Config *mapWithUnmarshalText `arg:"--config"` } err := parse("", &args) @@ -1489,22 +1489,22 @@ func TestTextUnmarshallerEmptyPointer(t *testing.T) { } // similar to the above but also implements MarshalText -type jsonMap2[T any] struct { - val T +type mapWithMarshalText struct { + val map[string]string } -func (v *jsonMap2[T]) MarshalText(data []byte) error { +func (v *mapWithMarshalText) MarshalText(data []byte) error { return json.Unmarshal(data, &v.val) } -func (v *jsonMap2[T]) UnmarshalText(data []byte) error { +func (v *mapWithMarshalText) UnmarshalText(data []byte) error { return json.Unmarshal(data, &v.val) } -func TestTextMarshallerUnmarshallerEmpty(t *testing.T) { +func TestTextMarshalerUnmarshalerEmpty(t *testing.T) { // based on https://github.com/alexflint/go-arg/issues/184 var args struct { - Config jsonMap2[map[string]string] `arg:"--config"` + Config mapWithMarshalText `arg:"--config"` } err := parse("", &args) @@ -1512,10 +1512,10 @@ func TestTextMarshallerUnmarshallerEmpty(t *testing.T) { assert.Empty(t, args.Config) } -func TestTextMarshallerUnmarshallerEmptyPointer(t *testing.T) { +func TestTextMarshalerUnmarshalerEmptyPointer(t *testing.T) { // a slight variant on https://github.com/alexflint/go-arg/issues/184 var args struct { - Config *jsonMap2[map[string]string] `arg:"--config"` + Config *mapWithMarshalText `arg:"--config"` } err := parse("", &args) From 27c832b934678d80580efd9940e5183ef3d687da Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 14:47:13 -0400 Subject: [PATCH 3/9] store both a default value and a string representation of that default value in the spec for each option --- parse.go | 130 +++++++++++++++++++++++++++----------------------- parse_test.go | 36 ++++++++++---- reflect.go | 8 ++-- usage_test.go | 2 +- 4 files changed, 102 insertions(+), 74 deletions(-) diff --git a/parse.go b/parse.go index 28ed014..980ac22 100644 --- a/parse.go +++ b/parse.go @@ -43,18 +43,19 @@ func (p path) Child(f reflect.StructField) path { // spec represents a command line option type spec struct { - dest path - field reflect.StructField // the struct field from which this option was created - long string // the --long form for this option, or empty if none - short string // the -s short form for this option, or empty if none - cardinality cardinality // determines how many tokens will be present (possible values: zero, one, multiple) - required bool // if true, this option must be present on the command line - positional bool // if true, this option will be looked for in the positional flags - separate bool // if true, each slice and map entry will have its own --flag - help string // the help text for this option - env string // the name of the environment variable for this option, or empty for none - defaultVal string // default value for this option - placeholder string // name of the data in help + dest path + field reflect.StructField // the struct field from which this option was created + long string // the --long form for this option, or empty if none + short string // the -s short form for this option, or empty if none + cardinality cardinality // determines how many tokens will be present (possible values: zero, one, multiple) + required bool // if true, this option must be present on the command line + positional bool // if true, this option will be looked for in the positional flags + separate bool // if true, each slice and map entry will have its own --flag + help string // the help text for this option + env string // the name of the environment variable for this option, or empty for none + defaultValue reflect.Value // default value for this option + defaultString string // default value for this option, in string form to be displayed in help text + placeholder string // name of the data in help } // command represents a named subcommand, or the top-level command @@ -210,39 +211,30 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { // for backwards compatibility, add nonzero field values as defaults for _, spec := range cmd.specs { - // do not read default when UnmarshalText is implemented but not MarshalText - if isTextUnmarshaler(spec.field.Type) && !isTextMarshaler(spec.field.Type) { - continue - } - - // do not process types that require multiple values - cardinality, _ := cardinalityOf(spec.field.Type) - if cardinality != one { - continue - } - // get the value v := p.val(spec.dest) if !v.IsValid() { continue } + // if the value is the "zero value" (e.g. nil pointer, empty struct) then ignore + if isZero(v) { + 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 { - if v.IsNil() { - continue - } s, err := m.MarshalText() if err != nil { return nil, fmt.Errorf("%v: error marshaling default value to string: %v", spec.dest, err) } - spec.defaultVal = string(s) - continue - } - - // finally, use the value as a default if it is non-zero - if !isZero(v) { - spec.defaultVal = fmt.Sprintf("%v", v) + spec.defaultString = string(s) + } else { + spec.defaultString = fmt.Sprintf("%v", v) } } @@ -311,11 +303,6 @@ 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 for _, key := range strings.Split(tag, ",") { @@ -342,11 +329,6 @@ 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 @@ -395,27 +377,60 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { spec.placeholder = strings.ToUpper(spec.field.Name) } - // Check whether this field is supported. It's good to do this here rather than + // 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 // exercised those fields. - if !isSubcommand { - cmd.specs = append(cmd.specs, &spec) + var err error + spec.cardinality, err = cardinalityOf(field.Type) + if err != nil { + errs = append(errs, fmt.Sprintf("%s.%s: %s fields are not supported", + t.Name(), field.Name, field.Type.String())) + return false + } - var err error - spec.cardinality, err = cardinalityOf(field.Type) - if err != nil { - errs = append(errs, fmt.Sprintf("%s.%s: %s fields are not supported", - t.Name(), field.Name, field.Type.String())) - return false - } - if spec.cardinality == multiple && hasDefault { + defaultString, hasDefault := field.Tag.Lookup("default") + if hasDefault { + // we do not support default values for maps and slices + if spec.cardinality == multiple { errs = append(errs, fmt.Sprintf("%s.%s: default values are not supported for slice or map fields", t.Name(), field.Name)) return false } + + // a required field cannot also have a default value + if spec.required { + errs = append(errs, fmt.Sprintf("%s.%s: 'required' cannot be used when a default value is specified", + t.Name(), field.Name)) + return false + } + + // parse the default value + spec.defaultString = defaultString + if field.Type.Kind() == reflect.Pointer { + // here we have a field of type *T and we create a new T, no need to dereference + // in order for the value to be settable + spec.defaultValue = reflect.New(field.Type.Elem()) + } else { + // here we have a field of type T and we create a new T and then dereference it + // so that the resulting value is settable + spec.defaultValue = reflect.New(field.Type).Elem() + } + err := scalar.ParseValue(spec.defaultValue, defaultString) + if err != nil { + errs = append(errs, fmt.Sprintf("%s.%s: error processing default value: %v", t.Name(), field.Name, err)) + return false + } } + // add the spec to the list of specs + cmd.specs = append(cmd.specs, &spec) + // if this was an embedded field then we already returned true up above return false }) @@ -682,11 +697,8 @@ func (p *Parser) process(args []string) error { } return errors.New(msg) } - 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) - } + if spec.defaultValue.IsValid() { + p.val(spec.dest).Set(spec.defaultValue) } } diff --git a/parse_test.go b/parse_test.go index 4dcb806..7747d05 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1321,13 +1321,21 @@ func TestDefaultOptionValues(t *testing.T) { require.NoError(t, err) assert.Equal(t, 123, args.A) - assert.Equal(t, 123, *args.B) + if assert.NotNil(t, args.B) { + assert.Equal(t, 123, *args.B) + } assert.Equal(t, "xyz", args.C) - assert.Equal(t, "abc", *args.D) + if assert.NotNil(t, args.D) { + 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) + if assert.NotNil(t, args.F) { + assert.Equal(t, 1.23, *args.F) + } assert.True(t, args.G) + if assert.NotNil(t, args.H) { + assert.True(t, *args.H) + } } func TestDefaultUnparseable(t *testing.T) { @@ -1336,7 +1344,7 @@ func TestDefaultUnparseable(t *testing.T) { } err := parse("", &args) - assert.EqualError(t, err, `error processing default value for --a: strconv.ParseInt: parsing "x": invalid syntax`) + assert.EqualError(t, err, `.A: error processing default value: strconv.ParseInt: parsing "x": invalid syntax`) } func TestDefaultPositionalValues(t *testing.T) { @@ -1355,13 +1363,21 @@ func TestDefaultPositionalValues(t *testing.T) { require.NoError(t, err) assert.Equal(t, 456, args.A) - assert.Equal(t, 789, *args.B) + if assert.NotNil(t, args.B) { + assert.Equal(t, 789, *args.B) + } assert.Equal(t, "abc", args.C) - assert.Equal(t, "abc", *args.D) + if assert.NotNil(t, args.D) { + 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) + if assert.NotNil(t, args.F) { + assert.Equal(t, 1.23, *args.F) + } assert.True(t, args.G) + if assert.NotNil(t, args.H) { + assert.True(t, *args.H) + } } func TestDefaultValuesNotAllowedWithRequired(t *testing.T) { @@ -1375,7 +1391,7 @@ func TestDefaultValuesNotAllowedWithRequired(t *testing.T) { func TestDefaultValuesNotAllowedWithSlice(t *testing.T) { var args struct { - A []int `default:"123"` // required not allowed with default! + A []int `default:"invalid"` // default values not allowed with slices } err := parse("", &args) diff --git a/reflect.go b/reflect.go index b87db2a..64c6343 100644 --- a/reflect.go +++ b/reflect.go @@ -16,9 +16,9 @@ var ( ) // cardinality tracks how many tokens are expected for a given spec -// - zero is a boolean, which does to expect any value -// - one is an ordinary option that will be parsed from a single token -// - multiple is a slice or map that can accept zero or more tokens +// - zero is a boolean, which does to expect any value +// - one is an ordinary option that will be parsed from a single token +// - multiple is a slice or map that can accept zero or more tokens type cardinality int const ( @@ -110,7 +110,7 @@ func isExported(field string) bool { // 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 || t.Kind() == reflect.Map { + if t.Kind() == reflect.Pointer || t.Kind() == reflect.Slice || t.Kind() == reflect.Map || t.Kind() == reflect.Chan || t.Kind() == reflect.Interface { return v.IsNil() } if !t.Comparable() { diff --git a/usage_test.go b/usage_test.go index 0a7ddd8..10072c1 100644 --- a/usage_test.go +++ b/usage_test.go @@ -474,7 +474,7 @@ Options: ShortOnly2 string `arg:"-b,--,required" help:"some help2"` } p, err := NewParser(Config{Program: "example"}, &args) - assert.NoError(t, err) + require.NoError(t, err) var help bytes.Buffer p.WriteHelp(&help) From 522dbbcea81814016fe121489fe9bc87ee1c82bf Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:08:48 -0400 Subject: [PATCH 4/9] add test for the new default value parsing logic as it shows up in help messages --- usage.go | 2 +- usage_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/usage.go b/usage.go index e936811..7d2a517 100644 --- a/usage.go +++ b/usage.go @@ -301,7 +301,7 @@ func (p *Parser) printOption(w io.Writer, spec *spec) { ways = append(ways, synopsis(spec, "-"+spec.short)) } if len(ways) > 0 { - printTwoCols(w, strings.Join(ways, ", "), spec.help, spec.defaultVal, spec.env) + printTwoCols(w, strings.Join(ways, ", "), spec.help, spec.defaultString, spec.env) } } diff --git a/usage_test.go b/usage_test.go index 10072c1..8fb32c8 100644 --- a/usage_test.go +++ b/usage_test.go @@ -601,3 +601,35 @@ error: something went wrong assert.Equal(t, expectedStdout[1:], b.String()) assert.Equal(t, -1, exitCode) } + +type lengthOf struct { + Length int +} + +func (p *lengthOf) UnmarshalText(b []byte) error { + p.Length = len(b) + return nil +} + +func TestHelpShowsDefaultValueFromOriginalTag(t *testing.T) { + // check that the usage text prints the original string from the default tag, not + // the serialization of the parsed value + + expectedHelp := ` +Usage: example [--test TEST] + +Options: + --test TEST [default: some_default_value] + --help, -h display this help and exit +` + + var args struct { + Test *lengthOf `default:"some_default_value"` + } + p, err := NewParser(Config{Program: "example"}, &args) + require.NoError(t, err) + + var help bytes.Buffer + p.WriteHelp(&help) + assert.Equal(t, expectedHelp[1:], help.String()) +} From 67f7183b85ccec77da44a823f419cfcd74d8c1e3 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:10:11 -0400 Subject: [PATCH 5/9] remove unused textMarshalerType and isTextMarshaler --- reflect.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/reflect.go b/reflect.go index 64c6343..466d65f 100644 --- a/reflect.go +++ b/reflect.go @@ -10,10 +10,7 @@ import ( scalar "github.com/alexflint/go-scalar" ) -var ( - textMarshalerType = reflect.TypeOf([]encoding.TextMarshaler{}).Elem() - textUnmarshalerType = reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem() -) +var textUnmarshalerType = reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem() // cardinality tracks how many tokens are expected for a given spec // - zero is a boolean, which does to expect any value @@ -91,11 +88,6 @@ func isBoolean(t reflect.Type) bool { } } -// isTextMarshaler returns true if the type or its pointer implements encoding.TextMarshaler -func isTextMarshaler(t reflect.Type) bool { - return t.Implements(textMarshalerType) || reflect.PtrTo(t).Implements(textMarshalerType) -} - // isTextUnmarshaler returns true if the type or its pointer implements encoding.TextUnmarshaler func isTextUnmarshaler(t reflect.Type) bool { return t.Implements(textUnmarshalerType) || reflect.PtrTo(t).Implements(textUnmarshalerType) From 9d5e97ac8ab96cab92076de4a1483d9018a3f9b4 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:12:53 -0400 Subject: [PATCH 6/9] drop unnecessary test --- parse.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/parse.go b/parse.go index 980ac22..cf364f3 100644 --- a/parse.go +++ b/parse.go @@ -213,9 +213,6 @@ func NewParser(config Config, dests ...interface{}) (*Parser, error) { for _, spec := range cmd.specs { // get the value v := p.val(spec.dest) - if !v.IsValid() { - continue - } // if the value is the "zero value" (e.g. nil pointer, empty struct) then ignore if isZero(v) { From d949871b676ed52669c17c2070dfb770b6ce28da Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:13:57 -0400 Subject: [PATCH 7/9] add further comment about backwards-compatible method for setting default values --- parse.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/parse.go b/parse.go index cf364f3..2fb7b1c 100644 --- a/parse.go +++ b/parse.go @@ -210,6 +210,8 @@ 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 { // get the value v := p.val(spec.dest) From 763072452fe0127e2002e12418d3e9c10ae1b8c0 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:21:21 -0400 Subject: [PATCH 8/9] use reflect.Ptr not reflect.Pointer since the latter was added in Go 1.18 --- parse.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse.go b/parse.go index dc48455..6ac5e99 100644 --- a/parse.go +++ b/parse.go @@ -416,7 +416,7 @@ func cmdFromStruct(name string, dest path, t reflect.Type) (*command, error) { // parse the default value spec.defaultString = defaultString - if field.Type.Kind() == reflect.Pointer { + if field.Type.Kind() == reflect.Ptr { // here we have a field of type *T and we create a new T, no need to dereference // in order for the value to be settable spec.defaultValue = reflect.New(field.Type.Elem()) From 3489ea5b2e9aa82dab4efc5e3f48fe6171f11ddd Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Sat, 29 Oct 2022 15:23:56 -0400 Subject: [PATCH 9/9] in a second place: use reflect.Ptr not reflect.Pointer since the latter was added in Go 1.18 --- reflect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflect.go b/reflect.go index 466d65f..5d6acb3 100644 --- a/reflect.go +++ b/reflect.go @@ -102,7 +102,7 @@ func isExported(field string) bool { // 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.Pointer || t.Kind() == reflect.Slice || t.Kind() == reflect.Map || t.Kind() == reflect.Chan || t.Kind() == reflect.Interface { + if t.Kind() == reflect.Ptr || t.Kind() == reflect.Slice || t.Kind() == reflect.Map || t.Kind() == reflect.Chan || t.Kind() == reflect.Interface { return v.IsNil() } if !t.Comparable() {