Bool flags in the paired `--foo` and `--no-foo` format (#346)

* Allow boolean flags in the matched `--foo` and `--no-foo` format.

* test for it

* Godoc for new functions

* Copyright date update

* Disallow user from setting either flag to false explicitly. Moar tests, for this & other cases

* Apply suggestions from code review

Co-Authored-By: Roland Huß <rhuss@redhat.com>

* Fixup

pkg/kn/core/root.go

@@ -29,6 +29,7 @@ import (
 	"github.com/knative/client/pkg/kn/commands/revision"
 	"github.com/knative/client/pkg/kn/commands/route"
 	"github.com/knative/client/pkg/kn/commands/service"
+	"github.com/knative/client/pkg/kn/flags"
 	homedir "github.com/mitchellh/go-homedir"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
@@ -117,6 +118,10 @@ func NewKnCommand(params ...commands.KnParams) *cobra.Command {
 
 		// Prevents Cobra from dealing with errors as we deal with them in main.go
 		SilenceErrors: true,
+
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
+			return flags.ReconcileBoolFlags(cmd.Flags())
+		},
 	}
 	if p.Output != nil {
 		rootCmd.SetOutput(p.Output)
@@ -125,7 +130,7 @@ func NewKnCommand(params ...commands.KnParams) *cobra.Command {
 	// Persistent flags
 	rootCmd.PersistentFlags().StringVar(&commands.CfgFile, "config", "", "kn config file (default is $HOME/.kn/config.yaml)")
 	rootCmd.PersistentFlags().StringVar(&p.KubeCfgPath, "kubeconfig", "", "kubectl config file (default is $HOME/.kube/config)")
-	rootCmd.PersistentFlags().BoolVar(&p.LogHttp, "log-http", false, "log http traffic")
+	flags.AddBothBoolFlags(rootCmd.PersistentFlags(), &p.LogHttp, "log-http", "", false, "log http traffic")
 
 	plugin.AddPluginFlags(rootCmd)
 	plugin.BindPluginsFlagToViper(rootCmd)

pkg/kn/flags/bool.go

@@ -0,0 +1,104 @@
+// Copyright © 2019 The Knative Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package flags
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/spf13/pflag"
+)
+
+var negPrefix = "no-"
+
+// AddBothBoolFlags adds the given flag in both `--foo` and `--no-foo` variants.
+// If you do this, make sure you call ReconcileBoolFlags later to catch errors and
+// set the relationship between the flag values.
+func AddBothBoolFlags(f *pflag.FlagSet, p *bool, name, short string, value bool, usage string) {
+
+	negativeName := negPrefix + name
+
+	f.BoolVarP(p, name, short, value, usage)
+	f.Bool(negativeName, !value, "do not "+usage)
+
+	if value {
+		err := f.MarkHidden(name)
+		if err != nil {
+			panic(err)
+		}
+	} else {
+		err := f.MarkHidden(negativeName)
+		if err != nil {
+			panic(err)
+		}
+	}
+}
+
+// ReconcileBoolFlags sets the value of the all the "--foo" flags based on
+// "--no-foo" if provided, and returns an error if both were provided or an
+// explicit value of false was provided to either (as that's confusing).
+func ReconcileBoolFlags(f *pflag.FlagSet) error {
+	var err error
+	f.VisitAll(func(flag *pflag.Flag) {
+		// Return early from our comprehension
+		if err != nil {
+			return
+		}
+		// Walk the "no-" versions of the flags. Make sure we didn't set
+		// both, and set the positive value to the opposite of the "no-"
+		// value if it exists.
+		if strings.HasPrefix(flag.Name, "no-") {
+			positiveName := flag.Name[len(negPrefix):]
+			positive := f.Lookup(positiveName)
+			if flag.Changed {
+				if positive.Changed {
+					err = fmt.Errorf("only one of --%s and --%s may be specified",
+						flag.Name, positiveName)
+					return
+				}
+				var noValue bool
+				noValue, err = strconv.ParseBool(flag.Value.String())
+				if err != nil {
+					return
+				}
+				if !noValue {
+					err = fmt.Errorf("use --%s instead of providing false to --%s",
+						positiveName, flag.Name)
+					if err != nil {
+						return
+					}
+				}
+				err = positive.Value.Set(strconv.FormatBool(!noValue))
+			} else if positive.Changed {
+				// For the positive version, just check it wasn't set to the
+				// confusing "false" value.
+				var yesValue bool
+				yesValue, err = strconv.ParseBool(positive.Value.String())
+				if err != nil {
+					return
+				}
+				if !yesValue {
+					err = fmt.Errorf("use --%s instead of providing false to --%s",
+						flag.Name, positiveName)
+					if err != nil {
+						return
+					}
+				}
+			}
+		}
+	})
+	return err
+}

pkg/kn/flags/bool_test.go

@@ -0,0 +1,71 @@
+// Copyright © 2019 The Knative Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package flags
+
+import (
+	"testing"
+
+	"github.com/spf13/pflag"
+	"gotest.tools/assert"
+)
+
+type boolPairTestCase struct {
+	name            string
+	defaultVal      bool
+	flags           []string
+	expectedResult  bool
+	expectedErrText string
+}
+
+func TestBooleanPair(t *testing.T) {
+	cases := []*boolPairTestCase{
+		{"foo", true, []string{}, true, ""},
+		{"foo", true, []string{"--foo"}, true, ""},
+		{"foo", true, []string{"--no-foo"}, false, ""},
+		{"foo", false, []string{"--foo"}, true, ""},
+		{"foo", false, []string{}, false, ""},
+		{"foo", false, []string{"--no-foo"}, false, ""},
+		{"foo", true, []string{"--foo", "--no-foo"}, false, "only one of"},
+		{"foo", true, []string{"--no-foo", "--foo"}, false, "only one of"},
+		// Disallow confusing "false" value.
+		{"foo", true, []string{"--foo=false"}, false, "use --no-foo instead of providing false to --foo"},
+		{"foo", true, []string{"--no-foo=false"}, false, "use --foo instead of providing false to --no-foo"},
+
+		// Ensure tests still pass if positive sorts after no- alphabetically.
+		{"zoo", true, []string{}, true, ""},
+		{"zoo", true, []string{"--zoo"}, true, ""},
+		{"zoo", true, []string{"--no-zoo"}, false, ""},
+		{"zoo", false, []string{"--zoo"}, true, ""},
+		{"zoo", false, []string{}, false, ""},
+		{"zoo", false, []string{"--no-zoo"}, false, ""},
+		{"zoo", true, []string{"--zoo", "--no-zoo"}, false, "only one of"},
+		{"zoo", true, []string{"--no-zoo", "--zoo"}, false, "only one of"},
+		// Disallow confusing "false" value.
+		{"zoo", true, []string{"--zoo=false"}, false, "use --no-zoo instead of providing false to --zoo"},
+		{"zoo", true, []string{"--no-zoo=false"}, false, "use --zoo instead of providing false to --no-zoo"},
+	}
+	for _, c := range cases {
+		f := &pflag.FlagSet{}
+		var result bool
+		AddBothBoolFlags(f, &result, c.name, "", c.defaultVal, "set "+c.name)
+		f.Parse(c.flags)
+		err := ReconcileBoolFlags(f)
+		if c.expectedErrText != "" {
+			assert.ErrorContains(t, err, c.expectedErrText)
+		} else {
+			assert.Equal(t, result, c.expectedResult)
+		}
+	}
+}