From 571d98044bfa47de64877bf1a1b4cc1f4d69471c Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Mon, 8 Feb 2016 09:53:51 -0700 Subject: [PATCH 1/7] Decouple OptionDefaulter from OptionDecoder --- command.go | 38 +++++++++++++++++++------- command_test.go | 26 ++++++------------ doc.go | 6 ++--- option.go | 72 +++++++++++++++++++------------------------------ 4 files changed, 66 insertions(+), 76 deletions(-) diff --git a/command.go b/command.go index e9be68e..c6db08d 100644 --- a/command.go +++ b/command.go @@ -136,7 +136,10 @@ func (c *Command) String() string { // parameters. func (c *Command) Decode(args []string) (path Path, positional []string, err error) { c.validate() - c.setDefaults() + err = c.setDefaults() + if err != nil { + return + } return parseArgs(c, args) } @@ -287,16 +290,27 @@ func (c *Command) validate() { } } -func (c *Command) setDefaults() { +func (c *Command) setDefaults() error { for _, opt := range c.Options { - defaulter, ok := opt.Decoder.(OptionDefaulter) - if ok { - defaulter.SetDefault() + if opt.Default == nil { + continue + } + val := opt.Default.Default() + if val == "" { + continue + } + err := opt.Decoder.Decode(val) + if err != nil { + return optionError{err: fmt.Errorf("Option %s initialized with invalid default value %q", opt, val)} } } for _, sub := range c.Subcommands { - sub.setDefaults() + err := sub.setDefaults() + if err != nil { + return err + } } + return nil } /* @@ -598,13 +612,17 @@ func parseOptionField(field reflect.StructField, fieldVal reflect.Value) *Option opt.Decoder = NewOptionDecoder(fieldVal.Addr().Interface()) } + var chain ChainedDefault + envName := field.Tag.Get(envTag) + if envName != "" { + chain = append(chain, NewEnvDefault(envName)) + } defaultArg := field.Tag.Get(defaultTag) if defaultArg != "" { - opt.Decoder = NewDefaulter(opt.Decoder, defaultArg) + chain = append(chain, StringDefault(defaultArg)) } - envName := field.Tag.Get(envTag) - if envName != "" { - opt.Decoder = NewEnvDefaulter(opt.Decoder, envName) + if len(chain) > 0 { + opt.Default = chain } opt.validate() diff --git a/command_test.go b/command_test.go index 0d5b414..0d36dc6 100644 --- a/command_test.go +++ b/command_test.go @@ -347,17 +347,17 @@ var defaultFieldTests = []defaultFieldTest{ // Field with an environment default {Args: []string{""}, Valid: true, Field: "EnvDefault", Value: 0}, {Args: []string{""}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "2", Field: "EnvDefault", Value: 2}, - {Args: []string{""}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "foo", Field: "EnvDefault", Value: 0}, {Args: []string{"-e", "4"}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "2", Field: "EnvDefault", Value: 4}, - {Args: []string{"-e", "4"}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "foo", Field: "EnvDefault", Value: 4}, + {Args: []string{""}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "foo"}, + {Args: []string{"-e", "4"}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "foo"}, {Args: []string{"-e", "foo"}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "2"}, // Field with both a default value and an environment default {Args: []string{""}, Valid: true, Field: "StackedDefault", Value: 84}, {Args: []string{""}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "2", Field: "StackedDefault", Value: 2}, - {Args: []string{""}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "foo", Field: "StackedDefault", Value: 84}, {Args: []string{"-s", "4"}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "2", Field: "StackedDefault", Value: 4}, - {Args: []string{"-s", "4"}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "foo", Field: "StackedDefault", Value: 4}, + {Args: []string{""}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, + {Args: []string{"-s", "4"}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, {Args: []string{"-s", "foo"}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, {Args: []string{"-s", "foo"}, Valid: false}, } @@ -405,21 +405,11 @@ func TestBogusDefaultField(t *testing.T) { BogusDefault int `option:"b" description:"An int field with a bogus default" default:"bogus"` }{} - defer func() { - r := recover() - if r != nil { - switch r.(type) { - case commandError, optionError: - // Intentional No-op - default: - panic(r) - } - } - }() - cmd := New("test", spec) - cmd.Decode([]string{}) - t.Errorf("Expected decoding to panic on bogus default value, but this didn't happen.") + _, _, err := cmd.Decode([]string{}) + if err == nil { + t.Errorf("Expected decoding error on bogus default value, but this didn't happen.") + } } /* diff --git a/doc.go b/doc.go index 02fcae6..89564eb 100644 --- a/doc.go +++ b/doc.go @@ -91,9 +91,7 @@ The New() function recognizes the following combinations of field tags: - description: the description to display for help output If both "default" and "env" are specified for an option field, the environment -variable is consulted first. If the environment variable is present and -decodes without error, that value is used. Otherwise, the value for the -"default" tag is used. Values specified via parsed arguments take precedence -over both types of defaults. +variable is consulted first. Values specified via parsed arguments take +precedence over both types of defaults. */ package writ diff --git a/option.go b/option.go index cd2a6e0..2260880 100644 --- a/option.go +++ b/option.go @@ -62,10 +62,11 @@ type Option struct { Decoder OptionDecoder // Optional - Flag bool // If set, the Option takes no arguments - Plural bool // If set, the Option may be specified multiple times - Description string // Options without descriptions are hidden - Placeholder string // Displayed next to option in help output (e.g. FILE) + Default OptionDefaulter // If set, Decoder.Decode() is called with Default.Default() prior to decoding args + Flag bool // If set, the Option takes no arguments + Plural bool // If set, the Option may be specified multiple times + Description string // Options without descriptions are hidden + Placeholder string // Displayed next to option in help output (e.g. FILE) } // ShortNames returns a filtered slice of the names that are exactly one rune in length. @@ -340,58 +341,41 @@ type flagAccumulator struct { value *int } -// OptionDefaulter initializes option values to defaults. If an OptionDecoder -// implements the OptionDefaulter interface, its SetDefault() method is called -// prior to decoding options. +// OptionDefaulter returns a default value for an Option type OptionDefaulter interface { - SetDefault() + Default() string } -// NewDefaulter builds an OptionDecoder that implements OptionDefaulter. -// SetDefault calls decoder.Decode() with the value of defaultArg. If the -// value fails to decode, SetDefault panics. -func NewDefaulter(decoder OptionDecoder, defaultArg string) OptionDecoder { - return defaulter{decoder, defaultArg} -} - -type defaulter struct { - OptionDecoder - defaultArg string -} +type StringDefault string -func (d defaulter) SetDefault() { - err := d.Decode(d.defaultArg) - if err != nil { - // Default values should be known correct values, so we panic on error - panicOption("error setting default value: decoder rejected arg %q", d.defaultArg) - } +func (d StringDefault) Default() string { + return string(d) } -// NewEnvDefaulter builds an OptionDecoder that implements OptionDefaulter. -// SetDefault calls decoder.Decode() with the value of the environment -// variable named by key. If the environment variable isn't set or fails to -// decode, SetDefault checks if decoder implements OptionDefault. If so, -// SetDefault calls decoder.SetDefault(). Otherwise, no action is taken. -func NewEnvDefaulter(decoder OptionDecoder, key string) OptionDecoder { - return envDefaulter{decoder, key} +// NewEnvDefault builds an OptionDefaulter that returns the value of the +// environment variable named by key when it's Default() method is called. +func NewEnvDefault(key string) OptionDefaulter { + return envDefaulter{key} } type envDefaulter struct { - OptionDecoder key string } -func (d envDefaulter) SetDefault() { - val := os.Getenv(d.key) - if val != "" { - err := d.Decode(val) - if err == nil { - return - } - } +func (d envDefaulter) Default() string { + return os.Getenv(d.key) +} - defaulter, ok := d.OptionDecoder.(OptionDefaulter) - if ok { - defaulter.SetDefault() +// ChainedDefault checks the Default() value of each element in it's slice +// and returns the first non-empty value, or "" if all values are empty. +type ChainedDefault []OptionDefaulter + +func (dc ChainedDefault) Default() string { + for _, defaulter := range dc { + value := defaulter.Default() + if value != "" { + return value + } } + return "" } From 2d2cbad19432d9eb064340a564b76cc7a443ec57 Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 08:41:17 -0700 Subject: [PATCH 2/7] Only default option values that need defaulting Only default option values for commands on the selected command path, and only if the options have not been supplied values via arguments. --- command.go | 52 ++++++++++++++++++++++++------------------------- command_test.go | 4 ++-- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/command.go b/command.go index c6db08d..95dd3ac 100644 --- a/command.go +++ b/command.go @@ -81,6 +81,15 @@ func (p Path) findOption(name string) *Option { return nil } +// options returns a slice of all options on the command path +func (p Path) options() []*Option { + var options []*Option + for _, command := range p { + options = append(options, command.Options...) + } + return options +} + // New reads the input spec, searching for fields tagged with "option", // "flag", or "command". The field type and tags are used to construct // a corresponding Command instance, which can be used to decode program @@ -136,10 +145,6 @@ func (c *Command) String() string { // parameters. func (c *Command) Decode(args []string) (path Path, positional []string, err error) { c.validate() - err = c.setDefaults() - if err != nil { - return - } return parseArgs(c, args) } @@ -290,29 +295,6 @@ func (c *Command) validate() { } } -func (c *Command) setDefaults() error { - for _, opt := range c.Options { - if opt.Default == nil { - continue - } - val := opt.Default.Default() - if val == "" { - continue - } - err := opt.Decoder.Decode(val) - if err != nil { - return optionError{err: fmt.Errorf("Option %s initialized with invalid default value %q", opt, val)} - } - } - for _, sub := range c.Subcommands { - err := sub.setDefaults() - if err != nil { - return err - } - } - return nil -} - /* * Argument parsing */ @@ -363,6 +345,22 @@ func parseArgs(c *Command, args []string) (path Path, positional []string, err e parseCmd = false positional = append(positional, a) } + + // Set defaults for unspecified options + for _, opt := range path.options() { + _, present := seen[opt] + if !present && opt.Default != nil { + val := opt.Default.Default() + if val == "" { + continue + } + err = opt.Decoder.Decode(val) + if err != nil { + err = optionError{err: fmt.Errorf("Option %s initialized with invalid default value %q", opt, val)} + return + } + } + } return } diff --git a/command_test.go b/command_test.go index 0d36dc6..20c645a 100644 --- a/command_test.go +++ b/command_test.go @@ -348,16 +348,16 @@ var defaultFieldTests = []defaultFieldTest{ {Args: []string{""}, Valid: true, Field: "EnvDefault", Value: 0}, {Args: []string{""}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "2", Field: "EnvDefault", Value: 2}, {Args: []string{"-e", "4"}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "2", Field: "EnvDefault", Value: 4}, + {Args: []string{"-e", "4"}, Valid: true, EnvKey: "ENV_DEFAULT", EnvValue: "foo", Field: "EnvDefault", Value: 4}, {Args: []string{""}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "foo"}, - {Args: []string{"-e", "4"}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "foo"}, {Args: []string{"-e", "foo"}, Valid: false, EnvKey: "ENV_DEFAULT", EnvValue: "2"}, // Field with both a default value and an environment default {Args: []string{""}, Valid: true, Field: "StackedDefault", Value: 84}, {Args: []string{""}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "2", Field: "StackedDefault", Value: 2}, {Args: []string{"-s", "4"}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "2", Field: "StackedDefault", Value: 4}, + {Args: []string{"-s", "4"}, Valid: true, EnvKey: "STACKED_DEFAULT", EnvValue: "foo", Field: "StackedDefault", Value: 4}, {Args: []string{""}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, - {Args: []string{"-s", "4"}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, {Args: []string{"-s", "foo"}, Valid: false, EnvKey: "STACKED_DEFAULT", EnvValue: "foo"}, {Args: []string{"-s", "foo"}, Valid: false}, } From 4b20445654865e5cfb589d83b140d16cafcf0a80 Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 08:47:28 -0700 Subject: [PATCH 3/7] Rename the OptionDefaulter interface to Defaulter Rename the OptionDefaulter interface to Defaulter. Now that it is Decoupled from the OptionDecoder interface, the old name feels wrong. Follow the more generic Go convention for single method interfaces. --- option.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/option.go b/option.go index 2260880..8e1bb6b 100644 --- a/option.go +++ b/option.go @@ -62,7 +62,7 @@ type Option struct { Decoder OptionDecoder // Optional - Default OptionDefaulter // If set, Decoder.Decode() is called with Default.Default() prior to decoding args + Default Defaulter // If set, Decoder.Decode() is called with Default.Default() prior to decoding args Flag bool // If set, the Option takes no arguments Plural bool // If set, the Option may be specified multiple times Description string // Options without descriptions are hidden @@ -341,8 +341,8 @@ type flagAccumulator struct { value *int } -// OptionDefaulter returns a default value for an Option -type OptionDefaulter interface { +// Defaulter returns a default value for an Option +type Defaulter interface { Default() string } @@ -352,9 +352,9 @@ func (d StringDefault) Default() string { return string(d) } -// NewEnvDefault builds an OptionDefaulter that returns the value of the +// NewEnvDefault builds a Defaulter that returns the value of the // environment variable named by key when it's Default() method is called. -func NewEnvDefault(key string) OptionDefaulter { +func NewEnvDefault(key string) Defaulter { return envDefaulter{key} } @@ -368,7 +368,7 @@ func (d envDefaulter) Default() string { // ChainedDefault checks the Default() value of each element in it's slice // and returns the first non-empty value, or "" if all values are empty. -type ChainedDefault []OptionDefaulter +type ChainedDefault []Defaulter func (dc ChainedDefault) Default() string { for _, defaulter := range dc { From 5c5f410cb32e8548fe89d142dd14f1a384b5f571 Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 08:51:48 -0700 Subject: [PATCH 4/7] Rename the method receiver for ChainedDefault --- option.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/option.go b/option.go index 8e1bb6b..0cb74e8 100644 --- a/option.go +++ b/option.go @@ -370,8 +370,8 @@ func (d envDefaulter) Default() string { // and returns the first non-empty value, or "" if all values are empty. type ChainedDefault []Defaulter -func (dc ChainedDefault) Default() string { - for _, defaulter := range dc { +func (d ChainedDefault) Default() string { + for _, defaulter := range d { value := defaulter.Default() if value != "" { return value From b15e4baff53801ecf12daa4d71d7579c5c8fa026 Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 08:54:53 -0700 Subject: [PATCH 5/7] Update comments on the Option type's fields --- option.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/option.go b/option.go index 0cb74e8..9f5e5ff 100644 --- a/option.go +++ b/option.go @@ -62,11 +62,11 @@ type Option struct { Decoder OptionDecoder // Optional - Default Defaulter // If set, Decoder.Decode() is called with Default.Default() prior to decoding args - Flag bool // If set, the Option takes no arguments - Plural bool // If set, the Option may be specified multiple times - Description string // Options without descriptions are hidden - Placeholder string // Displayed next to option in help output (e.g. FILE) + Default Defaulter // The Default value is used when no explicit value is provided + Flag bool // If set, the Option takes no arguments + Plural bool // If set, the Option may be specified multiple times + Description string // Options without descriptions are hidden + Placeholder string // Displayed next to option in help output (e.g. FILE) } // ShortNames returns a filtered slice of the names that are exactly one rune in length. From 2da5e60ae62ba7f3af4eeb0c679911fdff3580f8 Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 09:33:54 -0700 Subject: [PATCH 6/7] Add/update comments related to Defaulter changes --- option.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/option.go b/option.go index 9f5e5ff..6b5194b 100644 --- a/option.go +++ b/option.go @@ -341,13 +341,16 @@ type flagAccumulator struct { value *int } -// Defaulter returns a default value for an Option +// Defaulter returns a default value for an Option. type Defaulter interface { Default() string } +// StringDefault returns its value when Default() is called. This makes it easy to +// pass string values as defaults for the Option.Default field. type StringDefault string +// Default returns the string value of d func (d StringDefault) Default() string { return string(d) } @@ -370,6 +373,7 @@ func (d envDefaulter) Default() string { // and returns the first non-empty value, or "" if all values are empty. type ChainedDefault []Defaulter +// Default returns the first non-empty value for the elements in d, or "". func (d ChainedDefault) Default() string { for _, defaulter := range d { value := defaulter.Default() From df70050e3d3b6e6fbf204075b7e30969085ef6ab Mon Sep 17 00:00:00 2001 From: Bob Ziuchkovski Date: Wed, 10 Feb 2016 09:37:03 -0700 Subject: [PATCH 7/7] Update explicit example to show Option.Default usage --- README.md | 1 + example_explicit_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/README.md b/README.md index 76a046f..2054fbf 100644 --- a/README.md +++ b/README.md @@ -259,6 +259,7 @@ func main() { Names: []string{"bootloader"}, Description: "Use the specified bootloader (grub, grub2, or lilo)", Decoder: writ.NewOptionDecoder(&config.bootloader), + Default: writ.StringDefault("grub2"), Placeholder: "NAME", }) platform := cmd.GroupOptions("bootloader") diff --git a/example_explicit_test.go b/example_explicit_test.go index 01958b4..05d80fb 100644 --- a/example_explicit_test.go +++ b/example_explicit_test.go @@ -55,6 +55,7 @@ func Example_explicit() { Names: []string{"bootloader"}, Description: "Use the specified bootloader (grub, grub2, or lilo)", Decoder: writ.NewOptionDecoder(&config.bootloader), + Default: writ.StringDefault("grub2"), Placeholder: "NAME", }) platform := cmd.GroupOptions("bootloader")