Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test if flag is a bool before setting inFlag #615

Closed
wants to merge 2 commits into from

Conversation

gmwxio
Copy link

@gmwxio gmwxio commented Jan 16, 2018

Bug fix to get the following to work

cli --boolflag command

Currently command is attached to the boolean flag.
According to pflag
https://github.com/spf13/pflag#command-line-flag-syntax

// boolean or flags where the 'no option default value' is set
-f
-f=true
-abc
but
-b true is INVALID

Therefore boolean flags not set inFlag.

There seems to be inconsistencies is c.Flags() earlier on, hence adding the call to merge.
Also I don't understand why NoOptDefVal is not set for booleans as is appeared to be automatic by pflag.

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2018

CLA assistant check
All committers have signed the CLA.

@dnephin
Copy link
Contributor

dnephin commented Jan 16, 2018

Could you write a test that shows the bug?

@eparis
Copy link
Collaborator

eparis commented Jan 16, 2018

If think you are saying that you have: flag.Value.Type() == "bool" but that flag.NoOptDefVal == "" I believe that shouldn't happen... Can you write a test how that happened.

@gmwxio
Copy link
Author

gmwxio commented Jan 17, 2018

@dnephin @eparis take a look at
https://github.com/wxio/cobra/tree/pr615test/tests

The issue may involve the combination of

var rootCmd = &cobra.Command{
	TraverseChildren: true,
}

and

var subCmd = &cobra.Command{
	Use:                "sub",
	DisableFlagParsing: true,
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("sub called with args %v", args)
	},
}

@gmwxio
Copy link
Author

gmwxio commented Jan 17, 2018

see #616

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants