From 957f4eb69cc402bd26f8e5fcdc80c1f2b76f2666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Sun, 26 Nov 2023 16:49:29 +0200 Subject: [PATCH 1/7] Add general purpose config env var getter Similar to the existing one for active help, but generalized. --- completions.go | 29 +++++++++++++++++ completions_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/completions.go b/completions.go index b0e41df0c..22084f23b 100644 --- a/completions.go +++ b/completions.go @@ -17,6 +17,7 @@ package cobra import ( "fmt" "os" + "regexp" "strings" "sync" @@ -899,3 +900,31 @@ func CompError(msg string) { func CompErrorln(msg string) { CompError(fmt.Sprintf("%s\n", msg)) } + +// configEnvVarGlobalPrefix should not be changed: users will be using it explicitly. +const configEnvVarGlobalPrefix = "COBRA" + +var configEnvVarPrefixSubstRegexp = regexp.MustCompile(`[^A-Z0-9_]`) + +// configEnvVar returns the name of the program-specific configuration environment +// variable. It has the format _ where is the name of the +// root command in upper case, with all non-ASCII-alphanumeric characters replaced by `_`. +func configEnvVar(name, suffix string) string { + // This format should not be changed: users will be using it explicitly. + v := strings.ToUpper(fmt.Sprintf("%s_%s", name, suffix)) + v = configEnvVarPrefixSubstRegexp.ReplaceAllString(v, "_") + return v +} + +// GetEnvConfig returns the value of the configuration environment variable +// _ where is the name of the root command in upper +// case, with all non-ASCII-alphanumeric characters replaced by `_`. +// If the value is empty or not set, the value of the environment variable +// COBRA_ is returned instead. +func GetEnvConfig(cmd *Command, suffix string) string { + v := os.Getenv(configEnvVar(cmd.Root().Name(), suffix)) + if v == "" { + v = os.Getenv(configEnvVar(configEnvVarGlobalPrefix, suffix)) + } + return v +} diff --git a/completions_test.go b/completions_test.go index d5aee2501..213c17a83 100644 --- a/completions_test.go +++ b/completions_test.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "fmt" + "os" "strings" "sync" "testing" @@ -3517,3 +3518,78 @@ func TestGetFlagCompletion(t *testing.T) { }) } } + +func TestGetEnvConfig(t *testing.T) { + testCases := []struct { + use string + suffix string + cmdVar string + globalVar string + cmdVal string + globalVal string + expected string + }{ + { + use: "root", + suffix: "test", + cmdVar: "ROOT_TEST", + globalVar: "COBRA_TEST", + cmdVal: "cmd", + globalVal: "global", + expected: "cmd", + }, + { + use: "root", + suffix: "test", + cmdVar: "ROOT_TEST", + globalVar: "COBRA_TEST", + cmdVal: "", + globalVal: "global", + expected: "global", + }, + { + use: "root", + suffix: "test", + cmdVar: "ROOT_TEST", + globalVar: "COBRA_TEST", + cmdVal: "", + globalVal: "", + expected: "", + }, + { + use: "foo.bar", + suffix: "test", + cmdVar: "FOO_BAR_TEST", + globalVar: "COBRA_TEST", + cmdVal: "cmd", + globalVal: "global", + expected: "cmd", + }, + { + use: "quux-BAZ", + suffix: "test", + cmdVar: "QUUX_BAZ_TEST", + globalVar: "COBRA_TEST", + cmdVal: "cmd", + globalVal: "global", + expected: "cmd", + }, + } + + for _, tc := range testCases { + // Could make env handling cleaner with t.Setenv with Go >= 1.17 + func() { + err := os.Setenv(tc.cmdVar, tc.cmdVal) + defer assertNoErr(t, os.Unsetenv(tc.cmdVar)) + assertNoErr(t, err) + err = os.Setenv(tc.globalVar, tc.globalVal) + defer assertNoErr(t, os.Unsetenv(tc.globalVar)) + assertNoErr(t, err) + cmd := &Command{Use: tc.use} + got := GetEnvConfig(cmd, tc.suffix) + if got != tc.expected { + t.Errorf("expected: %q, got: %q", tc.expected, got) + } + }() + } +} From 97b70019e9a3e2618c895c0e93c3fc6d7101fc17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 24 Oct 2023 23:36:49 +0300 Subject: [PATCH 2/7] Make use of the general purpose config env var getter in active help --- active_help.go | 13 +++---------- completions.go | 10 +++++++--- site/content/completions/_index.md | 3 +++ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/active_help.go b/active_help.go index 5f965e057..25c30e3cc 100644 --- a/active_help.go +++ b/active_help.go @@ -17,21 +17,17 @@ package cobra import ( "fmt" "os" - "regexp" - "strings" ) const ( activeHelpMarker = "_activeHelp_ " // The below values should not be changed: programs will be using them explicitly // in their user documentation, and users will be using them explicitly. - activeHelpEnvVarSuffix = "_ACTIVE_HELP" - activeHelpGlobalEnvVar = "COBRA_ACTIVE_HELP" + activeHelpEnvVarSuffix = "ACTIVE_HELP" + activeHelpGlobalEnvVar = configEnvVarGlobalPrefix + "_" + activeHelpEnvVarSuffix activeHelpGlobalDisable = "0" ) -var activeHelpEnvVarPrefixSubstRegexp = regexp.MustCompile(`[^A-Z0-9_]`) - // AppendActiveHelp adds the specified string to the specified array to be used as ActiveHelp. // Such strings will be processed by the completion script and will be shown as ActiveHelp // to the user. @@ -60,8 +56,5 @@ func GetActiveHelpConfig(cmd *Command) string { // variable. It has the format _ACTIVE_HELP where is the name of the // root command in upper case, with all non-ASCII-alphanumeric characters replaced by `_`. func activeHelpEnvVar(name string) string { - // This format should not be changed: users will be using it explicitly. - activeHelpEnvVar := strings.ToUpper(fmt.Sprintf("%s%s", name, activeHelpEnvVarSuffix)) - activeHelpEnvVar = activeHelpEnvVarPrefixSubstRegexp.ReplaceAllString(activeHelpEnvVar, "_") - return activeHelpEnvVar + return configEnvVar(name, activeHelpEnvVarSuffix) } diff --git a/completions.go b/completions.go index 22084f23b..5070becc0 100644 --- a/completions.go +++ b/completions.go @@ -212,7 +212,7 @@ func (c *Command) initCompleteCmd(args []string) { // 2- Even without completions, we need to print the directive } - noDescriptions := (cmd.CalledAs() == ShellCompNoDescRequestCmd) + noDescriptions := cmd.CalledAs() == ShellCompNoDescRequestCmd || GetEnvConfig(cmd, configEnvVarSuffixDescriptions) == configEnvVarDescriptionsOff noActiveHelp := GetActiveHelpConfig(finalCmd) == activeHelpGlobalDisable out := finalCmd.OutOrStdout() for _, comp := range completions { @@ -901,8 +901,12 @@ func CompErrorln(msg string) { CompError(fmt.Sprintf("%s\n", msg)) } -// configEnvVarGlobalPrefix should not be changed: users will be using it explicitly. -const configEnvVarGlobalPrefix = "COBRA" +// These values should not be changed: users will be using them explicitly. +const ( + configEnvVarGlobalPrefix = "COBRA" + configEnvVarSuffixDescriptions = "COMPLETION_DESCRIPTIONS" + configEnvVarDescriptionsOff = "off" +) var configEnvVarPrefixSubstRegexp = regexp.MustCompile(`[^A-Z0-9_]`) diff --git a/site/content/completions/_index.md b/site/content/completions/_index.md index 4efad2907..b2b8017c6 100644 --- a/site/content/completions/_index.md +++ b/site/content/completions/_index.md @@ -393,6 +393,9 @@ $ source <(helm completion bash --no-descriptions) $ helm completion [tab][tab] bash fish powershell zsh ``` + +Setting the `_COMPLETION_DESCRIPTIONS` environment variable (falling back to `COBRA_COMPLETION_DESCRIPTIONS` if empty or not set) to `off` achieves the same. `` is the name of your program with all non-ASCII-alphanumeric characters replaced by `_`. + ## Bash completions ### Dependencies From 7aa559d693ce60d79c83942b31e1d0f76c7530bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 11 Dec 2023 17:37:09 +0200 Subject: [PATCH 3/7] Make getEnvConfig private --- completions.go | 6 +++--- completions_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/completions.go b/completions.go index 5070becc0..5788674ff 100644 --- a/completions.go +++ b/completions.go @@ -212,7 +212,7 @@ func (c *Command) initCompleteCmd(args []string) { // 2- Even without completions, we need to print the directive } - noDescriptions := cmd.CalledAs() == ShellCompNoDescRequestCmd || GetEnvConfig(cmd, configEnvVarSuffixDescriptions) == configEnvVarDescriptionsOff + noDescriptions := cmd.CalledAs() == ShellCompNoDescRequestCmd || getEnvConfig(cmd, configEnvVarSuffixDescriptions) == configEnvVarDescriptionsOff noActiveHelp := GetActiveHelpConfig(finalCmd) == activeHelpGlobalDisable out := finalCmd.OutOrStdout() for _, comp := range completions { @@ -920,12 +920,12 @@ func configEnvVar(name, suffix string) string { return v } -// GetEnvConfig returns the value of the configuration environment variable +// getEnvConfig returns the value of the configuration environment variable // _ where is the name of the root command in upper // case, with all non-ASCII-alphanumeric characters replaced by `_`. // If the value is empty or not set, the value of the environment variable // COBRA_ is returned instead. -func GetEnvConfig(cmd *Command, suffix string) string { +func getEnvConfig(cmd *Command, suffix string) string { v := os.Getenv(configEnvVar(cmd.Root().Name(), suffix)) if v == "" { v = os.Getenv(configEnvVar(configEnvVarGlobalPrefix, suffix)) diff --git a/completions_test.go b/completions_test.go index 213c17a83..c8af7e8f6 100644 --- a/completions_test.go +++ b/completions_test.go @@ -3586,7 +3586,7 @@ func TestGetEnvConfig(t *testing.T) { defer assertNoErr(t, os.Unsetenv(tc.globalVar)) assertNoErr(t, err) cmd := &Command{Use: tc.use} - got := GetEnvConfig(cmd, tc.suffix) + got := getEnvConfig(cmd, tc.suffix) if got != tc.expected { t.Errorf("expected: %q, got: %q", tc.expected, got) } From 138b0ab96495545094a0576d989241872687f886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 11 Dec 2023 18:04:40 +0200 Subject: [PATCH 4/7] Use strconv.ParseBool to parse descriptions state from env --- completions.go | 9 +++++++-- site/content/completions/_index.md | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/completions.go b/completions.go index 5788674ff..ad7b6d0a2 100644 --- a/completions.go +++ b/completions.go @@ -18,6 +18,7 @@ import ( "fmt" "os" "regexp" + "strconv" "strings" "sync" @@ -212,7 +213,12 @@ func (c *Command) initCompleteCmd(args []string) { // 2- Even without completions, we need to print the directive } - noDescriptions := cmd.CalledAs() == ShellCompNoDescRequestCmd || getEnvConfig(cmd, configEnvVarSuffixDescriptions) == configEnvVarDescriptionsOff + noDescriptions := cmd.CalledAs() == ShellCompNoDescRequestCmd + if !noDescriptions { + if doDescriptions, err := strconv.ParseBool(getEnvConfig(cmd, configEnvVarSuffixDescriptions)); err == nil { + noDescriptions = !doDescriptions + } + } noActiveHelp := GetActiveHelpConfig(finalCmd) == activeHelpGlobalDisable out := finalCmd.OutOrStdout() for _, comp := range completions { @@ -905,7 +911,6 @@ func CompErrorln(msg string) { const ( configEnvVarGlobalPrefix = "COBRA" configEnvVarSuffixDescriptions = "COMPLETION_DESCRIPTIONS" - configEnvVarDescriptionsOff = "off" ) var configEnvVarPrefixSubstRegexp = regexp.MustCompile(`[^A-Z0-9_]`) diff --git a/site/content/completions/_index.md b/site/content/completions/_index.md index b2b8017c6..02257ade2 100644 --- a/site/content/completions/_index.md +++ b/site/content/completions/_index.md @@ -394,7 +394,7 @@ $ helm completion [tab][tab] bash fish powershell zsh ``` -Setting the `_COMPLETION_DESCRIPTIONS` environment variable (falling back to `COBRA_COMPLETION_DESCRIPTIONS` if empty or not set) to `off` achieves the same. `` is the name of your program with all non-ASCII-alphanumeric characters replaced by `_`. +Setting the `_COMPLETION_DESCRIPTIONS` environment variable (falling back to `COBRA_COMPLETION_DESCRIPTIONS` if empty or not set) to a [falsey value](https://pkg.go.dev/strconv#ParseBool) achieves the same. `` is the name of your program with all non-ASCII-alphanumeric characters replaced by `_`. ## Bash completions From c96f1a622a4db0f6e407762be6852ea44247866d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 11 Dec 2023 18:06:40 +0200 Subject: [PATCH 5/7] Defer unsetting test env vars properly --- completions_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/completions_test.go b/completions_test.go index c8af7e8f6..8e1c138f5 100644 --- a/completions_test.go +++ b/completions_test.go @@ -3580,10 +3580,14 @@ func TestGetEnvConfig(t *testing.T) { // Could make env handling cleaner with t.Setenv with Go >= 1.17 func() { err := os.Setenv(tc.cmdVar, tc.cmdVal) - defer assertNoErr(t, os.Unsetenv(tc.cmdVar)) + defer func() { + assertNoErr(t, os.Unsetenv(tc.cmdVar)) + }() assertNoErr(t, err) err = os.Setenv(tc.globalVar, tc.globalVal) - defer assertNoErr(t, os.Unsetenv(tc.globalVar)) + defer func() { + assertNoErr(t, os.Unsetenv(tc.globalVar)) + }() assertNoErr(t, err) cmd := &Command{Use: tc.use} got := getEnvConfig(cmd, tc.suffix) From 9740ecead4dbfd90f985a0c9d8d5d7a0cf5e9212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 11 Dec 2023 18:22:19 +0200 Subject: [PATCH 6/7] Distinguish env var getter test cases better --- completions_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/completions_test.go b/completions_test.go index 8e1c138f5..291e657cc 100644 --- a/completions_test.go +++ b/completions_test.go @@ -3521,6 +3521,7 @@ func TestGetFlagCompletion(t *testing.T) { func TestGetEnvConfig(t *testing.T) { testCases := []struct { + desc string use string suffix string cmdVar string @@ -3530,6 +3531,7 @@ func TestGetEnvConfig(t *testing.T) { expected string }{ { + desc: "Command envvar overrides global", use: "root", suffix: "test", cmdVar: "ROOT_TEST", @@ -3539,6 +3541,7 @@ func TestGetEnvConfig(t *testing.T) { expected: "cmd", }, { + desc: "Missing/empty command envvar falls back to global", use: "root", suffix: "test", cmdVar: "ROOT_TEST", @@ -3548,6 +3551,7 @@ func TestGetEnvConfig(t *testing.T) { expected: "global", }, { + desc: "Missing/empty command and global envvars fall back to empty", use: "root", suffix: "test", cmdVar: "ROOT_TEST", @@ -3557,6 +3561,7 @@ func TestGetEnvConfig(t *testing.T) { expected: "", }, { + desc: "Periods in command use transform to underscores in env var name", use: "foo.bar", suffix: "test", cmdVar: "FOO_BAR_TEST", @@ -3566,6 +3571,7 @@ func TestGetEnvConfig(t *testing.T) { expected: "cmd", }, { + desc: "Dashes in command use transform to underscores in env var name", use: "quux-BAZ", suffix: "test", cmdVar: "QUUX_BAZ_TEST", @@ -3577,8 +3583,8 @@ func TestGetEnvConfig(t *testing.T) { } for _, tc := range testCases { - // Could make env handling cleaner with t.Setenv with Go >= 1.17 - func() { + t.Run(tc.desc, func(t *testing.T) { + // Could make env handling cleaner with t.Setenv with Go >= 1.17 err := os.Setenv(tc.cmdVar, tc.cmdVal) defer func() { assertNoErr(t, os.Unsetenv(tc.cmdVar)) @@ -3594,6 +3600,6 @@ func TestGetEnvConfig(t *testing.T) { if got != tc.expected { t.Errorf("expected: %q, got: %q", tc.expected, got) } - }() + }) } } From 1107319c750f917bc3bf1c74a7705fe6e93123be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 11 Dec 2023 22:18:46 +0200 Subject: [PATCH 7/7] Add description disabling test cases --- completions_test.go | 106 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/completions_test.go b/completions_test.go index 291e657cc..df153fcf2 100644 --- a/completions_test.go +++ b/completions_test.go @@ -3603,3 +3603,109 @@ func TestGetEnvConfig(t *testing.T) { }) } } + +func TestDisableDescriptions(t *testing.T) { + rootCmd := &Command{ + Use: "root", + Run: emptyRun, + } + + childCmd := &Command{ + Use: "thechild", + Short: "The child command", + Run: emptyRun, + } + rootCmd.AddCommand(childCmd) + + specificDescriptionsEnvVar := configEnvVar(rootCmd.Name(), configEnvVarSuffixDescriptions) + globalDescriptionsEnvVar := configEnvVar(configEnvVarGlobalPrefix, configEnvVarSuffixDescriptions) + + const ( + descLineWithDescription = "first\tdescription" + descLineWithoutDescription = "first" + ) + childCmd.ValidArgsFunction = func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) { + comps := []string{descLineWithDescription} + return comps, ShellCompDirectiveDefault + } + + testCases := []struct { + desc string + globalEnvValue string + specificEnvValue string + expectedLine string + }{ + { + "No env variables set", + "", + "", + descLineWithDescription, + }, + { + "Global value false", + "false", + "", + descLineWithoutDescription, + }, + { + "Specific value false", + "", + "false", + descLineWithoutDescription, + }, + { + "Both values false", + "false", + "false", + descLineWithoutDescription, + }, + { + "Both values true", + "true", + "true", + descLineWithDescription, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + if err := os.Setenv(specificDescriptionsEnvVar, tc.specificEnvValue); err != nil { + t.Errorf("Unexpected error setting %s: %v", specificDescriptionsEnvVar, err) + } + if err := os.Setenv(globalDescriptionsEnvVar, tc.globalEnvValue); err != nil { + t.Errorf("Unexpected error setting %s: %v", globalDescriptionsEnvVar, err) + } + + var run = func() { + output, err := executeCommand(rootCmd, ShellCompRequestCmd, "thechild", "") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + expected := strings.Join([]string{ + tc.expectedLine, + ":0", + "Completion ended with directive: ShellCompDirectiveDefault", ""}, "\n") + if output != expected { + t.Errorf("expected: %q, got: %q", expected, output) + } + } + + run() + + // For empty cases, test also unset state + if tc.specificEnvValue == "" { + if err := os.Unsetenv(specificDescriptionsEnvVar); err != nil { + t.Errorf("Unexpected error unsetting %s: %v", specificDescriptionsEnvVar, err) + } + run() + } + if tc.globalEnvValue == "" { + if err := os.Unsetenv(globalDescriptionsEnvVar); err != nil { + t.Errorf("Unexpected error unsetting %s: %v", globalDescriptionsEnvVar, err) + } + run() + } + }) + } +}