Improve common mistake when using Cobra flagsets

Users get tripped up when accidentally mix & matching
Cobra's `Flags()` and `PersistentFlags()`, where they will
call `Lookup(<flag>)` on the wrong flagset.

Following the README, users would call `Lookup` to fill in
the flag source parameter of Viper's `BindPFlag`, but by
using the wrong flagset, the parameter would simply be nil.
This would then result in a runtime panic whenever trying
to retrieve the flag's value with `Get(<flag>)`.

This work alleviates the mistake by:
+ Performing a nil check in `BindPFlag` and causing
  `BindFlagValue` to return an error if the parameter
  flag is nil, as it would be during the expected mixup.
+ Updating the godoc of `BindPFlag` and the README's example,
  to drill the fact that `Flags` and `PersistentFlags` are
  different flagsets.
+ Adding a test for this pattern of binding nil on accident
  and then retrieving flags. Before these changes, this test
  would panic. Now, the test instead gracefully errors and
  applies no changes to the viper instance.
This commit is contained in:
Tyler Butters 2017-10-08 12:47:12 -04:00
parent d9cca5ef33
commit 9e7b3a1bb5
3 changed files with 21 additions and 1 deletions

View file

@ -246,6 +246,11 @@ serverCmd.Flags().Int("port", 1138, "Port to run Application server on")
viper.BindPFlag("port", serverCmd.Flags().Lookup("port"))
```
The above example would also work with
[Cobra's Persistent Flags](https://github.com/spf13/cobra#persistent-flags),
but careful not to mix and match Cobra `Flags` and `PersistentFlags` when
binding with Viper.
You can also bind an existing set of pflags (pflag.FlagSet):
Example:

View file

@ -799,9 +799,15 @@ func (v *Viper) BindPFlags(flags *pflag.FlagSet) error {
// serverCmd.Flags().Int("port", 1138, "Port to run Application server on")
// Viper.BindPFlag("port", serverCmd.Flags().Lookup("port"))
//
// NOTE: Be consistent with either Cobra's Flags() or PersistentFlags(),
// do not mix them.
func BindPFlag(key string, flag *pflag.Flag) error { return v.BindPFlag(key, flag) }
func (v *Viper) BindPFlag(key string, flag *pflag.Flag) error {
return v.BindFlagValue(key, pflagValue{flag})
var flagVal FlagValue
if flag != nil {
flagVal = pflagValue{flag}
}
return v.BindFlagValue(key, flagVal)
}
// BindFlagValues binds a full FlagValue set to the configuration, using each flag's long

View file

@ -597,6 +597,15 @@ func TestBindPFlag(t *testing.T) {
}
func TestBindPFlagToNil(t *testing.T) {
v := New()
// nil arg often comes as an accident from calling something like:
// `myCobraCmd.Flags().Lookup("missingFlag")`
err := v.BindPFlag("host", nil)
assert.Equal(t, nil, v.Get("host")) // `Get` here used to panic
assert.EqualError(t, err, `flag for "host" is nil`)
}
func TestBoundCaseSensitivity(t *testing.T) {
assert.Equal(t, "brown", Get("eyes"))