From 5f0c48f092161d61d13a2ebeda134c51c2d5b2d9 Mon Sep 17 00:00:00 2001 From: Alex Flint Date: Tue, 4 Oct 2022 11:54:22 -0700 Subject: [PATCH] move construction logic out of parse.go into construct.go --- v2/construct.go | 317 +++++++++++++++++++++++++++++++++ v2/construct_test.go | 25 +++ v2/parse.go | 412 ++++++------------------------------------- v2/parse_test.go | 27 +-- 4 files changed, 399 insertions(+), 382 deletions(-) create mode 100644 v2/construct.go create mode 100644 v2/construct_test.go diff --git a/v2/construct.go b/v2/construct.go new file mode 100644 index 0000000..bed64eb --- /dev/null +++ b/v2/construct.go @@ -0,0 +1,317 @@ +package arg + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "reflect" + "strings" +) + +// Argument represents a command line argument +type Argument 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 +} + +// Command represents a named subcommand, or the top-level command +type Command struct { + name string + help string + dest path + args []*Argument + subcommands []*Command + parent *Command +} + +// Parser represents a set of command line options with destination values +type Parser struct { + cmd *Command // the top-level command + root reflect.Value // destination struct to fill will values + version string // version from the argument struct + prologue string // prologue for help text (from the argument struct) + epilogue string // epilogue for help text (from the argument struct) + + // the following fields are updated during processing of command line arguments + leaf *Command // the subcommand we processed last + accumulatedArgs []*Argument // concatenation of the leaf subcommand's arguments plus all ancestors' arguments + seen map[*Argument]bool // the arguments we encountered while processing command line arguments +} + +// Versioned is the interface that the destination struct should implement to +// make a version string appear at the top of the help message. +type Versioned interface { + // Version returns the version string that will be printed on a line by itself + // at the top of the help message. + Version() string +} + +// Described is the interface that the destination struct should implement to +// make a description string appear at the top of the help message. +type Described interface { + // Description returns the string that will be printed on a line by itself + // at the top of the help message. + Description() string +} + +// Epilogued is the interface that the destination struct should implement to +// add an epilogue string at the bottom of the help message. +type Epilogued interface { + // Epilogue returns the string that will be printed on a line by itself + // at the end of the help message. + Epilogue() string +} + +// the ParserOption interface matches options for the parser constructor +type ParserOption interface { + parserOption() +} + +type programNameParserOption struct { + s string +} + +func (programNameParserOption) parserOption() {} + +// WithProgramName overrides the name of the program as displayed in help test +func WithProgramName(name string) ParserOption { + return programNameParserOption{s: name} +} + +// NewParser constructs a parser from a list of destination structs +func NewParser(dest interface{}, options ...ParserOption) (*Parser, error) { + // check the destination type + t := reflect.TypeOf(dest) + if t.Kind() != reflect.Ptr { + panic(fmt.Sprintf("%s is not a pointer (did you forget an ampersand?)", t)) + } + + // pick a program name for help text and usage output + program := "program" + if len(os.Args) > 0 { + program = filepath.Base(os.Args[0]) + } + + // apply the options + for _, opt := range options { + switch opt := opt.(type) { + case programNameParserOption: + program = opt.s + } + } + + // build the root command from the struct + cmd, err := cmdFromStruct(program, path{}, t) + if err != nil { + return nil, err + } + + // construct the parser + p := Parser{ + seen: make(map[*Argument]bool), + root: reflect.ValueOf(dest), + cmd: cmd, + } + + // check for version, prologue, and epilogue + if dest, ok := dest.(Versioned); ok { + p.version = dest.Version() + } + if dest, ok := dest.(Described); ok { + p.prologue = dest.Description() + } + if dest, ok := dest.(Epilogued); ok { + p.epilogue = dest.Epilogue() + } + + return &p, nil +} + +func cmdFromStruct(name string, dest path, t reflect.Type) (*Command, 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()) + } + + 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, + } + + var errs []string + walkFields(t, func(field reflect.StructField, t reflect.Type) bool { + // check for the ignore switch in the tag + tag := field.Tag.Get("arg") + if tag == "-" { + return false + } + + // if this is an embedded struct then recurse into its fields, even if + // it is unexported, because exported fields on unexported embedded + // structs are still writable + if field.Anonymous && field.Type.Kind() == reflect.Struct { + return true + } + + // ignore any other unexported field + if !isExported(field.Name) { + return false + } + + // duplicate the entire path to avoid slice overwrites + subdest := dest.Child(field) + arg := Argument{ + dest: subdest, + field: field, + long: strings.ToLower(field.Name), + } + + help, exists := field.Tag.Lookup("help") + if exists { + arg.help = help + } + + defaultVal, hasDefault := field.Tag.Lookup("default") + if hasDefault { + arg.defaultVal = defaultVal + } + + // Look at the tag + var isSubcommand bool // tracks whether this field is a subcommand + for _, key := range strings.Split(tag, ",") { + if key == "" { + continue + } + key = strings.TrimLeft(key, " ") + var value string + if pos := strings.Index(key, ":"); pos != -1 { + value = key[pos+1:] + key = key[:pos] + } + + switch { + case strings.HasPrefix(key, "---"): + errs = append(errs, fmt.Sprintf("%s.%s: too many hyphens", t.Name(), field.Name)) + case strings.HasPrefix(key, "--"): + arg.long = key[2:] + case strings.HasPrefix(key, "-"): + if len(key) != 2 { + errs = append(errs, fmt.Sprintf("%s.%s: short arguments must be one character only", + t.Name(), field.Name)) + return false + } + arg.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 + } + arg.required = true + case key == "positional": + arg.positional = true + case key == "separate": + arg.separate = true + case key == "help": // deprecated + arg.help = value + case key == "env": + // Use override name if provided + if value != "" { + arg.env = value + } else { + arg.env = strings.ToUpper(field.Name) + } + case key == "subcommand": + // decide on a name for the subcommand + cmdname := value + if cmdname == "" { + cmdname = strings.ToLower(field.Name) + } + + // parse the subcommand recursively + subcmd, err := cmdFromStruct(cmdname, subdest, 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 + default: + errs = append(errs, fmt.Sprintf("unrecognized tag '%s' on field %s", key, tag)) + return false + } + } + + placeholder, hasPlaceholder := field.Tag.Lookup("placeholder") + if hasPlaceholder { + arg.placeholder = placeholder + } else if arg.long != "" { + arg.placeholder = strings.ToUpper(arg.long) + } else { + arg.placeholder = strings.ToUpper(arg.field.Name) + } + + // 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.args = append(cmd.args, &arg) + + var err error + arg.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 arg.cardinality == multiple && hasDefault { + errs = append(errs, fmt.Sprintf("%s.%s: default values are not supported for slice or map fields", + t.Name(), field.Name)) + return false + } + } + + // if this was an embedded field then we already returned true up above + return false + }) + + if len(errs) > 0 { + return nil, errors.New(strings.Join(errs, "\n")) + } + + // check that we don't have both positionals and subcommands + var hasPositional bool + for _, arg := range cmd.args { + if arg.positional { + hasPositional = true + } + } + if hasPositional && len(cmd.subcommands) > 0 { + return nil, fmt.Errorf("%s cannot have both subcommands and positional arguments", dest) + } + + return &cmd, nil +} diff --git a/v2/construct_test.go b/v2/construct_test.go new file mode 100644 index 0000000..66b2f80 --- /dev/null +++ b/v2/construct_test.go @@ -0,0 +1,25 @@ +package arg + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInvalidTag(t *testing.T) { + var args struct { + Foo string `arg:"this_is_not_valid"` + } + _, err := NewParser(&args) + assert.Error(t, err) +} + +func TestUnexportedFieldsSkipped(t *testing.T) { + var args struct { + unexported struct{} + } + + _, err := NewParser(&args) + require.NoError(t, err) +} diff --git a/v2/parse.go b/v2/parse.go index f5bcab4..1a29e35 100644 --- a/v2/parse.go +++ b/v2/parse.go @@ -5,65 +5,12 @@ import ( "errors" "fmt" "os" - "path/filepath" "reflect" "strings" scalar "github.com/alexflint/go-scalar" ) -// path represents a sequence of steps to find the output location for an -// argument or subcommand in the final destination struct -type path struct { - fields []reflect.StructField // sequence of struct fields to traverse -} - -// String gets a string representation of the given path -func (p path) String() string { - s := "args" - for _, f := range p.fields { - s += "." + f.Name - } - return s -} - -// Child gets a new path representing a child of this path. -func (p path) Child(f reflect.StructField) path { - // copy the entire slice of fields to avoid possible slice overwrite - subfields := make([]reflect.StructField, len(p.fields)+1) - copy(subfields, p.fields) - subfields[len(subfields)-1] = f - return path{ - fields: subfields, - } -} - -// Arg represents a command line argument -type Argument 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 -} - -// Command represents a named subcommand, or the top-level command -type Command struct { - name string - help string - dest path - args []*Argument - subcommands []*Command - parent *Command -} - // ErrHelp indicates that -h or --help were provided var ErrHelp = errors.New("help requested by user") @@ -103,308 +50,6 @@ func Parse(dest interface{}, options ...ParserOption) error { return p.Parse(os.Args, os.Environ()) } -// Parser represents a set of command line options with destination values -type Parser struct { - cmd *Command // the top-level command - root reflect.Value // destination struct to fill will values - version string // version from the argument struct - prologue string // prologue for help text (from the argument struct) - epilogue string // epilogue for help text (from the argument struct) - - // the following fields are updated during processing of command line arguments - leaf *Command // the subcommand we processed last - accumulatedArgs []*Argument // concatenation of the leaf subcommand's arguments plus all ancestors' arguments - seen map[*Argument]bool // the arguments we encountered while processing command line arguments -} - -// Versioned is the interface that the destination struct should implement to -// make a version string appear at the top of the help message. -type Versioned interface { - // Version returns the version string that will be printed on a line by itself - // at the top of the help message. - Version() string -} - -// Described is the interface that the destination struct should implement to -// make a description string appear at the top of the help message. -type Described interface { - // Description returns the string that will be printed on a line by itself - // at the top of the help message. - Description() string -} - -// Epilogued is the interface that the destination struct should implement to -// add an epilogue string at the bottom of the help message. -type Epilogued interface { - // Epilogue returns the string that will be printed on a line by itself - // at the end of the help message. - Epilogue() string -} - -// walkFields calls a function for each field of a struct, recursively expanding struct fields. -func walkFields(t reflect.Type, visit func(field reflect.StructField, owner reflect.Type) bool) { - walkFieldsImpl(t, visit, nil) -} - -func walkFieldsImpl(t reflect.Type, visit func(field reflect.StructField, owner reflect.Type) bool, path []int) { - for i := 0; i < t.NumField(); i++ { - field := t.Field(i) - field.Index = make([]int, len(path)+1) - copy(field.Index, append(path, i)) - expand := visit(field, t) - if expand && field.Type.Kind() == reflect.Struct { - var subpath []int - if field.Anonymous { - subpath = append(path, i) - } - walkFieldsImpl(field.Type, visit, subpath) - } - } -} - -// the ParserOption interface matches options for the parser constructor -type ParserOption interface { - parserOption() -} - -type programNameParserOption struct { - s string -} - -func (programNameParserOption) parserOption() {} - -// WithProgramName overrides the name of the program as displayed in help test -func WithProgramName(name string) ParserOption { - return programNameParserOption{s: name} -} - -// NewParser constructs a parser from a list of destination structs -func NewParser(dest interface{}, options ...ParserOption) (*Parser, error) { - // check the destination type - t := reflect.TypeOf(dest) - if t.Kind() != reflect.Ptr { - panic(fmt.Sprintf("%s is not a pointer (did you forget an ampersand?)", t)) - } - - // pick a program name for help text and usage output - program := "program" - if len(os.Args) > 0 { - program = filepath.Base(os.Args[0]) - } - - // apply the options - for _, opt := range options { - switch opt := opt.(type) { - case programNameParserOption: - program = opt.s - } - } - - // build the root command from the struct - cmd, err := cmdFromStruct(program, path{}, t) - if err != nil { - return nil, err - } - - // construct the parser - p := Parser{ - seen: make(map[*Argument]bool), - root: reflect.ValueOf(dest), - cmd: cmd, - } - - // check for version, prologue, and epilogue - if dest, ok := dest.(Versioned); ok { - p.version = dest.Version() - } - if dest, ok := dest.(Described); ok { - p.prologue = dest.Description() - } - if dest, ok := dest.(Epilogued); ok { - p.epilogue = dest.Epilogue() - } - - return &p, nil -} - -func cmdFromStruct(name string, dest path, t reflect.Type) (*Command, 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()) - } - - 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, - } - - var errs []string - walkFields(t, func(field reflect.StructField, t reflect.Type) bool { - // check for the ignore switch in the tag - tag := field.Tag.Get("arg") - if tag == "-" { - return false - } - - // if this is an embedded struct then recurse into its fields, even if - // it is unexported, because exported fields on unexported embedded - // structs are still writable - if field.Anonymous && field.Type.Kind() == reflect.Struct { - return true - } - - // ignore any other unexported field - if !isExported(field.Name) { - return false - } - - // duplicate the entire path to avoid slice overwrites - subdest := dest.Child(field) - arg := Argument{ - dest: subdest, - field: field, - long: strings.ToLower(field.Name), - } - - help, exists := field.Tag.Lookup("help") - if exists { - arg.help = help - } - - defaultVal, hasDefault := field.Tag.Lookup("default") - if hasDefault { - arg.defaultVal = defaultVal - } - - // Look at the tag - var isSubcommand bool // tracks whether this field is a subcommand - for _, key := range strings.Split(tag, ",") { - if key == "" { - continue - } - key = strings.TrimLeft(key, " ") - var value string - if pos := strings.Index(key, ":"); pos != -1 { - value = key[pos+1:] - key = key[:pos] - } - - switch { - case strings.HasPrefix(key, "---"): - errs = append(errs, fmt.Sprintf("%s.%s: too many hyphens", t.Name(), field.Name)) - case strings.HasPrefix(key, "--"): - arg.long = key[2:] - case strings.HasPrefix(key, "-"): - if len(key) != 2 { - errs = append(errs, fmt.Sprintf("%s.%s: short arguments must be one character only", - t.Name(), field.Name)) - return false - } - arg.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 - } - arg.required = true - case key == "positional": - arg.positional = true - case key == "separate": - arg.separate = true - case key == "help": // deprecated - arg.help = value - case key == "env": - // Use override name if provided - if value != "" { - arg.env = value - } else { - arg.env = strings.ToUpper(field.Name) - } - case key == "subcommand": - // decide on a name for the subcommand - cmdname := value - if cmdname == "" { - cmdname = strings.ToLower(field.Name) - } - - // parse the subcommand recursively - subcmd, err := cmdFromStruct(cmdname, subdest, 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 - default: - errs = append(errs, fmt.Sprintf("unrecognized tag '%s' on field %s", key, tag)) - return false - } - } - - placeholder, hasPlaceholder := field.Tag.Lookup("placeholder") - if hasPlaceholder { - arg.placeholder = placeholder - } else if arg.long != "" { - arg.placeholder = strings.ToUpper(arg.long) - } else { - arg.placeholder = strings.ToUpper(arg.field.Name) - } - - // 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.args = append(cmd.args, &arg) - - var err error - arg.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 arg.cardinality == multiple && hasDefault { - errs = append(errs, fmt.Sprintf("%s.%s: default values are not supported for slice or map fields", - t.Name(), field.Name)) - return false - } - } - - // if this was an embedded field then we already returned true up above - return false - }) - - if len(errs) > 0 { - return nil, errors.New(strings.Join(errs, "\n")) - } - - // check that we don't have both positionals and subcommands - var hasPositional bool - for _, arg := range cmd.args { - if arg.positional { - hasPositional = true - } - } - if hasPositional && len(cmd.subcommands) > 0 { - return nil, fmt.Errorf("%s cannot have both subcommands and positional arguments", dest) - } - - return &cmd, nil -} - // Parse processes the given command line option, storing the results in the field // of the structs from which NewParser was constructed func (p *Parser) Parse(args, env []string) error { @@ -763,11 +408,6 @@ func (p *Parser) Validate() error { return nil } -// isFlag returns true if a token is a flag such as "-v" or "--user" but not "-" or "--" -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 { @@ -785,6 +425,58 @@ func (p *Parser) val(dest path) reflect.Value { return v } +// path represents a sequence of steps to find the output location for an +// argument or subcommand in the final destination struct +type path struct { + fields []reflect.StructField // sequence of struct fields to traverse +} + +// String gets a string representation of the given path +func (p path) String() string { + s := "args" + for _, f := range p.fields { + s += "." + f.Name + } + return s +} + +// Child gets a new path representing a child of this path. +func (p path) Child(f reflect.StructField) path { + // copy the entire slice of fields to avoid possible slice overwrite + subfields := make([]reflect.StructField, len(p.fields)+1) + copy(subfields, p.fields) + subfields[len(subfields)-1] = f + return path{ + fields: subfields, + } +} + +// walkFields calls a function for each field of a struct, recursively expanding struct fields. +func walkFields(t reflect.Type, visit func(field reflect.StructField, owner reflect.Type) bool) { + walkFieldsImpl(t, visit, nil) +} + +func walkFieldsImpl(t reflect.Type, visit func(field reflect.StructField, owner reflect.Type) bool, path []int) { + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + field.Index = make([]int, len(path)+1) + copy(field.Index, append(path, i)) + expand := visit(field, t) + if expand && field.Type.Kind() == reflect.Struct { + var subpath []int + if field.Anonymous { + subpath = append(path, i) + } + walkFieldsImpl(field.Type, visit, subpath) + } + } +} + +// isFlag returns true if a token is a flag such as "-v" or "--user" but not "-" or "--" +func isFlag(s string) bool { + return strings.HasPrefix(s, "-") && strings.TrimLeft(s, "-") != "" +} + // findOption finds an option from its name, or returns nil if no arg is found func findOption(args []*Argument, name string) *Argument { for _, arg := range args { diff --git a/v2/parse_test.go b/v2/parse_test.go index c65ded8..ee46006 100644 --- a/v2/parse_test.go +++ b/v2/parse_test.go @@ -629,14 +629,6 @@ func TestParse(t *testing.T) { assert.Equal(t, "bar", args.Foo) } -func TestParseError(t *testing.T) { - var args struct { - Foo string `arg:"this_is_not_valid"` - } - _, err := NewParser(&args) - assert.Error(t, err) -} - func TestMustParse(t *testing.T) { var args struct { Foo string @@ -795,13 +787,13 @@ func TestEnvironmentVariableIgnored(t *testing.T) { } func TestDefaultValuesIgnored(t *testing.T) { - var args struct { - Foo string `default:"bad"` - } - - // just checking that default values are not automatically applied + // check that default values are not automatically applied // in ProcessCommandLine or ProcessEnvironment + var args struct { + Foo string `default:"hello"` + } + p, err := NewParser(&args) require.NoError(t, err) @@ -1390,15 +1382,6 @@ func TestDefaultValuesNotAllowedWithSlice(t *testing.T) { assert.EqualError(t, err, ".A: default values are not supported for slice or map fields") } -func TestUnexportedFieldsSkipped(t *testing.T) { - var args struct { - unexported struct{} - } - - _, err := NewParser(&args) - require.NoError(t, err) -} - func TestMustParseInvalidParser(t *testing.T) { originalExit := osExit originalStdout := stdout