From 21dd7001f27196b7bda3ca931943f0ac5895faf1 Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Tue, 10 Mar 2020 11:20:18 +0100 Subject: [PATCH 01/11] feat(core): positional argument --- internal/core/arg_specs.go | 16 ++++++ internal/core/cobra_utils.go | 64 +++++++++++++++++++++ internal/core/cobra_utils_test.go | 95 ++++++++++++++++++++++++++++++- 3 files changed, 174 insertions(+), 1 deletion(-) diff --git a/internal/core/arg_specs.go b/internal/core/arg_specs.go index 7c2c8fe10c..a7195f5bf2 100644 --- a/internal/core/arg_specs.go +++ b/internal/core/arg_specs.go @@ -8,6 +8,15 @@ import ( type ArgSpecs []*ArgSpec +func (s ArgSpecs) GetPositionalArg() *ArgSpec { + for _, spec := range s { + if spec.Positional { + return spec + } + } + return nil +} + func (s ArgSpecs) GetByName(name string) *ArgSpec { for _, spec := range s { if spec.Name == name { @@ -62,6 +71,13 @@ type ArgSpec struct { // ValidateFunc validates an argument. ValidateFunc ArgSpecValidateFunc + + // Positional defines whether the argument is a positional parameter. + Positional bool +} + +func (a *ArgSpec) Prefix() string { + return a.Name + "=" } type DefaultFunc func() (value string, doc string) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index 971ab0a07e..e9ec6cb982 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -25,6 +25,11 @@ func cobraRun(ctx context.Context, cmd *Command) func(*cobra.Command, []string) // unmarshalled arguments will be store in this interface cmdArgs := reflect.New(cmd.ArgsType).Interface() + // Handle positional argument by catching first argument `` and rewrite it to `=` + if err = handlePositionalArg(cmd, rawArgs); err != nil { + return err + } + // Apply default values on missing args. rawArgs = ApplyDefaultValues(cmd.ArgSpecs, rawArgs) @@ -78,6 +83,65 @@ func cobraRun(ctx context.Context, cmd *Command) func(*cobra.Command, []string) } } +// handlePositionalArg will catch positional argument if command has one. +// When a positional argument is found it will mutate its value in rawArgs to match the argument unmarshaller format. +// E.g.: '[value b=true c=1]' will be mutated to '[a=value b=true c=1]'. +// It returns errors when: +// - no positional argument is found. +// - a positional argument exists, but is not positional. +// - an argument duplicates a positional argument. +func handlePositionalArg(cmd *Command, rawArgs []string) error { + positionalArg := cmd.ArgSpecs.GetPositionalArg() + + // Command does not have a positional argument. + if positionalArg == nil { + return nil + } + + // Positional argument is found condition. + positionalArgumentFound := len(rawArgs) > 0 && !strings.Contains(rawArgs[0], "=") + + // Argument exists but is not positional. + for i, arg := range rawArgs { + if strings.HasPrefix(arg, positionalArg.Prefix()) { + return &CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: positionalArgHint(cmd, strings.TrimLeft(arg, positionalArg.Prefix()), append(rawArgs[:i], rawArgs[i+1:]...), positionalArgumentFound), + } + } + } + + // If positional argument is found, prefix it with `arg-name=`. + if positionalArgumentFound { + rawArgs[0] = positionalArg.Prefix() + rawArgs[0] + return nil + } + + // No positional argument found. + return &CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: positionalArgHint(cmd, "<"+positionalArg.Name+">", rawArgs, false), + } +} + +// positionalArgHint helps formatting the positional argument error. +func positionalArgHint(cmd *Command, hintValue string, otherArgs []string, positionalArgumentFound bool) string { + suggestedArgs := []string{} + + // If no positional argument exists, suggest one. + if !positionalArgumentFound { + suggestedArgs = append(suggestedArgs, hintValue) + } + + // Suggest to use the other arguments. + for _, arg := range otherArgs { + suggestedArgs = append(suggestedArgs, arg) + } + + suggestedCommand := append([]string{"scw", cmd.getPath()}, suggestedArgs...) + return "Try running '" + strings.Join(suggestedCommand, " ") + "'." +} + func handleUnmarshalErrors(cmd *Command, unmarshalErr *args.UnmarshalArgError) error { wrappedErr := errors.Unwrap(unmarshalErr) diff --git a/internal/core/cobra_utils_test.go b/internal/core/cobra_utils_test.go index ebd80e53b8..1c6a4d64ae 100644 --- a/internal/core/cobra_utils_test.go +++ b/internal/core/cobra_utils_test.go @@ -8,7 +8,8 @@ import ( ) type testType struct { - Name string + NameID string + Tag string } func testGetCommands() *Commands { @@ -25,6 +26,22 @@ func testGetCommands() *Commands { return "", nil }, }, + &Command{ + Namespace: "test-positional", + ArgSpecs: ArgSpecs{ + { + Name: "name-id", + Positional: true, + }, + { + Name: "tag", + }, + }, + ArgsType: reflect.TypeOf(testType{}), + Run: func(ctx context.Context, argsI interface{}) (i interface{}, e error) { + return "", nil + }, + }, ) } @@ -53,3 +70,79 @@ func Test_handleUnmarshalErrors(t *testing.T) { ), })) } + +func Test_PositionalArg(t *testing.T) { + t.Run("Error", func(t *testing.T) { + t.Run("Missing1", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional", + Check: TestCheckCombine( + TestCheckExitCode(1), + TestCheckError(&CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: "Try running 'scw test-positional '.", + }), + ), + })) + + t.Run("Missing2", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional tag=world", + Check: TestCheckCombine( + TestCheckExitCode(1), + TestCheckError(&CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: "Try running 'scw test-positional tag=world'.", + }), + ), + })) + + t.Run("Invalid1", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional name-id=plop tag=world", + Check: TestCheckCombine( + TestCheckExitCode(1), + TestCheckError(&CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: "Try running 'scw test-positional plop tag=world'.", + }), + ), + })) + + t.Run("Invalid2", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional tag=world name-id=plop", + Check: TestCheckCombine( + TestCheckExitCode(1), + TestCheckError(&CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: fmt.Sprintf("Try running 'scw test-positional plop tag=world'."), + }), + ), + })) + + t.Run("Invalid3", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional plop name-id=plop", + Check: TestCheckCombine( + TestCheckExitCode(1), + TestCheckError(&CliError{ + Err: fmt.Errorf("a positional argument is required for this command"), + Hint: fmt.Sprintf("Try running 'scw test-positional plop'."), + }), + ), + })) + }) + + t.Run("simple", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional plop", + Check: TestCheckExitCode(0), + })) + + t.Run("full command", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional plop tag=world", + Check: TestCheckExitCode(0), + })) +} From 0d27e716842b8a3f7b8ed551dcee7218749028a9 Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Tue, 10 Mar 2020 11:27:48 +0100 Subject: [PATCH 02/11] fix: help for positional arg --- internal/core/cobra_builder.go | 3 +++ internal/core/cobra_utils_test.go | 9 +++++++++ ...st-positional-arg-full-command#01.stderr.golden | 14 ++++++++++++++ 3 files changed, 26 insertions(+) create mode 100644 internal/core/testdata/test-positional-arg-full-command#01.stderr.golden diff --git a/internal/core/cobra_builder.go b/internal/core/cobra_builder.go index 15dc13c9ed..67a7f1f4f9 100644 --- a/internal/core/cobra_builder.go +++ b/internal/core/cobra_builder.go @@ -127,6 +127,9 @@ func (b *cobraBuilder) hydrateCobra(cobraCmd *cobra.Command, cmd *Command) { if cobraCmd.HasAvailableSubCommands() || len(cobraCmd.Commands()) > 0 { cobraCmd.Annotations["CommandUsage"] += " " } + if positionalArg := cmd.ArgSpecs.GetPositionalArg(); positionalArg != nil { + cobraCmd.Annotations["CommandUsage"] += " <" + positionalArg.Name + ">" + } if cobraCmd.HasAvailableLocalFlags() || cobraCmd.HasAvailableFlags() || cobraCmd.LocalFlags() != nil { cobraCmd.Annotations["CommandUsage"] += " [flags]" } diff --git a/internal/core/cobra_utils_test.go b/internal/core/cobra_utils_test.go index 1c6a4d64ae..75a507e346 100644 --- a/internal/core/cobra_utils_test.go +++ b/internal/core/cobra_utils_test.go @@ -145,4 +145,13 @@ func Test_PositionalArg(t *testing.T) { Cmd: "scw test-positional plop tag=world", Check: TestCheckExitCode(0), })) + + t.Run("full command", Test(&TestConfig{ + Commands: testGetCommands(), + Cmd: "scw test-positional -h", + Check: TestCheckCombine( + TestCheckExitCode(0), + TestCheckGolden(), + ), + })) } diff --git a/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden b/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden new file mode 100644 index 0000000000..0cf2acea38 --- /dev/null +++ b/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden @@ -0,0 +1,14 @@ +USAGE: + scw [global-flags] test-positional [flags] [arg=value ...] + +ARGS: + [name-id] + [tag] + +FLAGS: + -h, --help help for test-positional + +GLOBAL FLAGS: + -D, --debug Enable debug mode + -o, --output string Output format: json or human + -p, --profile string The config profile to use From 177780e27161afd06b0579322cacbdcb9a9a2934 Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Tue, 10 Mar 2020 11:34:56 +0100 Subject: [PATCH 03/11] address gosimple comment --- internal/core/cobra_utils.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index e9ec6cb982..9d7d271b75 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -134,9 +134,7 @@ func positionalArgHint(cmd *Command, hintValue string, otherArgs []string, posit } // Suggest to use the other arguments. - for _, arg := range otherArgs { - suggestedArgs = append(suggestedArgs, arg) - } + suggestedArgs = append(suggestedArgs, otherArgs...) suggestedCommand := append([]string{"scw", cmd.getPath()}, suggestedArgs...) return "Try running '" + strings.Join(suggestedCommand, " ") + "'." From 2bb3c7fd01b7ac15920edd2167e7552f99aae275 Mon Sep 17 00:00:00 2001 From: Olivier Cano Date: Tue, 10 Mar 2020 17:28:00 +0100 Subject: [PATCH 04/11] Update internal/core/cobra_utils.go Co-Authored-By: Quentin Brosse --- internal/core/cobra_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index 9d7d271b75..b19893c2b8 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -25,7 +25,7 @@ func cobraRun(ctx context.Context, cmd *Command) func(*cobra.Command, []string) // unmarshalled arguments will be store in this interface cmdArgs := reflect.New(cmd.ArgsType).Interface() - // Handle positional argument by catching first argument `` and rewrite it to `=` + // Handle positional argument by catching first argument `` and rewrite it to `=`. if err = handlePositionalArg(cmd, rawArgs); err != nil { return err } From 0f86f6a485cba697cee86c2932deac2b619807fc Mon Sep 17 00:00:00 2001 From: Olivier Cano Date: Tue, 10 Mar 2020 17:28:09 +0100 Subject: [PATCH 05/11] Update internal/core/cobra_utils.go Co-Authored-By: Quentin Brosse --- internal/core/cobra_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index b19893c2b8..87f7efe91b 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -88,7 +88,7 @@ func cobraRun(ctx context.Context, cmd *Command) func(*cobra.Command, []string) // E.g.: '[value b=true c=1]' will be mutated to '[a=value b=true c=1]'. // It returns errors when: // - no positional argument is found. -// - a positional argument exists, but is not positional. +// - an unknown positional argument exists in the comand. // - an argument duplicates a positional argument. func handlePositionalArg(cmd *Command, rawArgs []string) error { positionalArg := cmd.ArgSpecs.GetPositionalArg() From 46015301fb9a7b570d2bb6d9291c30fc67765b9e Mon Sep 17 00:00:00 2001 From: Olivier Cano Date: Tue, 10 Mar 2020 17:28:22 +0100 Subject: [PATCH 06/11] Update internal/core/cobra_utils.go Co-Authored-By: Quentin Brosse --- internal/core/cobra_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index 87f7efe91b..bc5d9941c1 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -124,7 +124,7 @@ func handlePositionalArg(cmd *Command, rawArgs []string) error { } } -// positionalArgHint helps formatting the positional argument error. +// positionalArgHint formats the positional argument error hint. func positionalArgHint(cmd *Command, hintValue string, otherArgs []string, positionalArgumentFound bool) string { suggestedArgs := []string{} From ae8b445de61dc133941afcfaf5e4276ffa471715 Mon Sep 17 00:00:00 2001 From: Olivier Cano Date: Tue, 10 Mar 2020 17:29:32 +0100 Subject: [PATCH 07/11] Update internal/core/cobra_utils.go Co-Authored-By: Quentin Brosse --- internal/core/cobra_utils.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index bc5d9941c1..2a72298566 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -104,9 +104,11 @@ func handlePositionalArg(cmd *Command, rawArgs []string) error { // Argument exists but is not positional. for i, arg := range rawArgs { if strings.HasPrefix(arg, positionalArg.Prefix()) { + argumentValue := strings.TrimLeft(arg, positionalArg.Prefix()) + otherArgs := append(rawArgs[:i], rawArgs[i+1:]...) return &CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: positionalArgHint(cmd, strings.TrimLeft(arg, positionalArg.Prefix()), append(rawArgs[:i], rawArgs[i+1:]...), positionalArgumentFound), + Hint: positionalArgHint(cmd, argumentValue, otherArgs, positionalArgumentFound), } } } From 3222569d03e46fec9a8ce39947edb8f7dca52d95 Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Tue, 10 Mar 2020 17:42:52 +0100 Subject: [PATCH 08/11] address quentin's comment --- internal/core/cobra_utils.go | 2 +- internal/core/cobra_utils_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index 2a72298566..3a00416f04 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -139,7 +139,7 @@ func positionalArgHint(cmd *Command, hintValue string, otherArgs []string, posit suggestedArgs = append(suggestedArgs, otherArgs...) suggestedCommand := append([]string{"scw", cmd.getPath()}, suggestedArgs...) - return "Try running '" + strings.Join(suggestedCommand, " ") + "'." + return "Try running: " + strings.Join(suggestedCommand, " ") } func handleUnmarshalErrors(cmd *Command, unmarshalErr *args.UnmarshalArgError) error { diff --git a/internal/core/cobra_utils_test.go b/internal/core/cobra_utils_test.go index 75a507e346..2e54fb1048 100644 --- a/internal/core/cobra_utils_test.go +++ b/internal/core/cobra_utils_test.go @@ -80,7 +80,7 @@ func Test_PositionalArg(t *testing.T) { TestCheckExitCode(1), TestCheckError(&CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: "Try running 'scw test-positional '.", + Hint: "Try running: scw test-positional ", }), ), })) @@ -92,7 +92,7 @@ func Test_PositionalArg(t *testing.T) { TestCheckExitCode(1), TestCheckError(&CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: "Try running 'scw test-positional tag=world'.", + Hint: "Try running: scw test-positional tag=world", }), ), })) @@ -104,7 +104,7 @@ func Test_PositionalArg(t *testing.T) { TestCheckExitCode(1), TestCheckError(&CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: "Try running 'scw test-positional plop tag=world'.", + Hint: "Try running: scw test-positional plop tag=world", }), ), })) @@ -116,7 +116,7 @@ func Test_PositionalArg(t *testing.T) { TestCheckExitCode(1), TestCheckError(&CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: fmt.Sprintf("Try running 'scw test-positional plop tag=world'."), + Hint: fmt.Sprintf("Try running: scw test-positional plop tag=world"), }), ), })) @@ -128,7 +128,7 @@ func Test_PositionalArg(t *testing.T) { TestCheckExitCode(1), TestCheckError(&CliError{ Err: fmt.Errorf("a positional argument is required for this command"), - Hint: fmt.Sprintf("Try running 'scw test-positional plop'."), + Hint: fmt.Sprintf("Try running: scw test-positional plop"), }), ), })) From ee4be8b2f9f0af1ef2aa4bea6e1255defe5fdac6 Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Tue, 10 Mar 2020 17:48:15 +0100 Subject: [PATCH 09/11] forbid 2 positional args --- internal/core/arg_specs.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/core/arg_specs.go b/internal/core/arg_specs.go index a7195f5bf2..d8d2db0cbe 100644 --- a/internal/core/arg_specs.go +++ b/internal/core/arg_specs.go @@ -9,12 +9,16 @@ import ( type ArgSpecs []*ArgSpec func (s ArgSpecs) GetPositionalArg() *ArgSpec { + var positionalArg *ArgSpec for _, spec := range s { if spec.Positional { - return spec + if positionalArg != nil { + panic(fmt.Errorf("more than one positional parameter detected: %s and %s are flagged as positional arg", positionalArg.Name, spec.Name)) + } + positionalArg = spec } } - return nil + return positionalArg } func (s ArgSpecs) GetByName(name string) *ArgSpec { From dd63baff9739fdcb16f59f37e4760678ebb3678d Mon Sep 17 00:00:00 2001 From: Olivier Cano Date: Wed, 11 Mar 2020 10:32:15 +0100 Subject: [PATCH 10/11] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Loïc Bourgois --- internal/core/arg_specs.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/core/arg_specs.go b/internal/core/arg_specs.go index d8d2db0cbe..df714bc6ed 100644 --- a/internal/core/arg_specs.go +++ b/internal/core/arg_specs.go @@ -10,12 +10,12 @@ type ArgSpecs []*ArgSpec func (s ArgSpecs) GetPositionalArg() *ArgSpec { var positionalArg *ArgSpec - for _, spec := range s { - if spec.Positional { + for _, argSpec := range s { + if argSpec.Positional { if positionalArg != nil { - panic(fmt.Errorf("more than one positional parameter detected: %s and %s are flagged as positional arg", positionalArg.Name, spec.Name)) + panic(fmt.Errorf("more than one positional parameter detected: %s and %s are flagged as positional arg", positionalArg.Name, argSpec.Name)) } - positionalArg = spec + positionalArg = argSpec } } return positionalArg @@ -76,7 +76,7 @@ type ArgSpec struct { // ValidateFunc validates an argument. ValidateFunc ArgSpecValidateFunc - // Positional defines whether the argument is a positional parameter. + // Positional defines whether the argument is a positional argument. Positional bool } From d8f24e4d58fd5836faf78432df796f40b4eb5dac Mon Sep 17 00:00:00 2001 From: Olivier CANO Date: Wed, 11 Mar 2020 10:38:39 +0100 Subject: [PATCH 11/11] document positional to be required --- internal/core/arg_specs.go | 2 +- internal/core/cobra_usage_builder.go | 2 +- .../test-positional-arg-full-command#01.stderr.golden | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/core/arg_specs.go b/internal/core/arg_specs.go index df714bc6ed..1160137700 100644 --- a/internal/core/arg_specs.go +++ b/internal/core/arg_specs.go @@ -76,7 +76,7 @@ type ArgSpec struct { // ValidateFunc validates an argument. ValidateFunc ArgSpecValidateFunc - // Positional defines whether the argument is a positional argument. + // Positional defines whether the argument is a positional argument. NB: a positional argument is required. Positional bool } diff --git a/internal/core/cobra_usage_builder.go b/internal/core/cobra_usage_builder.go index fba3ad9bd0..5db19de8fe 100644 --- a/internal/core/cobra_usage_builder.go +++ b/internal/core/cobra_usage_builder.go @@ -59,7 +59,7 @@ func _buildUsageArgs(w io.Writer, argSpecs ArgSpecs) error { _, doc := argSpec.Default() argSpecUsageLeftPart = fmt.Sprintf("%s=%s", argSpecUsageLeftPart, doc) } - if !argSpec.Required { + if !argSpec.Required && !argSpec.Positional { argSpecUsageLeftPart = fmt.Sprintf("[%s]", argSpecUsageLeftPart) } diff --git a/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden b/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden index 0cf2acea38..7e121af744 100644 --- a/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden +++ b/internal/core/testdata/test-positional-arg-full-command#01.stderr.golden @@ -2,8 +2,8 @@ USAGE: scw [global-flags] test-positional [flags] [arg=value ...] ARGS: - [name-id] - [tag] + name-id + [tag] FLAGS: -h, --help help for test-positional