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.

Sign up for GitHub

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

Jump to bottom

Share more code between Traverse and stripFlags #617

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+51 −28
Open

Share more code between Traverse and stripFlags #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1195d1
mergePersistentFlags in Traverse
gmwxio Jan 17, 2018
46f41fc
Refactor to re-use more code between Traverse and stripFlags()
dnephin Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 30 additions & 28 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,30 +436,7 @@ func (c *Command) VersionTemplate() string {
`
}

func hasNoOptDefVal(name string, fs *flag.FlagSet) bool {
flag := fs.Lookup(name)
if flag == nil {
return false
}
return flag.NoOptDefVal != ""
}

func shortHasNoOptDefVal(name string, fs *flag.FlagSet) bool {
if len(name) == 0 {
return false
}

flag := fs.ShorthandLookup(name[:1])
if flag == nil {
return false
}
return flag.NoOptDefVal != ""
}

func stripFlags(args []string, c *Command) []string {
if len(args) == 0 {
return args
}
c.mergePersistentFlags()

commands := []string{}
Expand All @@ -470,11 +447,11 @@ Loop:
s := args[0]
args = args[1:]
switch {
case strings.HasPrefix(s, "--") && !strings.Contains(s, "=") && !hasNoOptDefVal(s[2:], flags):
case strings.HasPrefix(s, "--") && !strings.Contains(s, "=") && !hasDefaultValue(s[2:], flags):
// If '--flag arg' then
// delete arg from args.
fallthrough // (do the same as below)
case strings.HasPrefix(s, "-") && !strings.Contains(s, "=") && len(s) == 2 && !shortHasNoOptDefVal(s[1:], flags):
case isShortFlagWithNoDefaultValue(s, flags):
// If '-f arg' then
// delete 'arg' from args or break the loop if len(args) <= 1.
if len(args) <= 1 {
Expand All @@ -491,6 +468,30 @@ Loop:
return commands
}

func hasDefaultValue(name string, fs *flag.FlagSet) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the right name? I know it is shorter/pithier than NoOptDefVal but isn't default value actually the value set at declaration?

cmd.Flags().StringVar(&str, "mystring", "default value", "My string variable")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, true. It's actually checking to see if the Value type used by the flag has a default, so maybe flagValueHasDefault would be more correct.

flag := fs.Lookup(name)
if flag == nil {
return false
}
return flag.NoOptDefVal != ""
}

func isShortFlagWithNoDefaultValue(arg string, flags *flag.FlagSet) bool {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this func isShortFlagWithNoDefaultValue is confusing.

partly copied from pr 615

func hasDefaultShort_s(arg string, c *Command) bool { // should probably return (bool, error)
	if strings.HasPrefix(arg, "-") {
		if !strings.Contains(arg, "=") {
                       // needs to change to a range to deal with -abcd goldfish
			if len(arg) == 2 {
				flag := c.Flags().ShorthandLookup(arg[1:])
				if flag == nil {
					return false
				}
				//shortHasNoOptDefVal
				return flag.NoOptDefVal != ""
			}
		}
  	}
	return false
 }

It's also a good place to solve the -abcd goldfish issue.

I think some of the issues and confusion are created as this code can't be sure if a flag exists and have to choose a default. Is it possible to

  1. change these helper funcs to return (bool, error), or
  2. check the flags before hand (might be hard as this code is figure out which flags need checking)

A lot of the if flag == nil { and if len(name) == 0 { code could be removed making this code clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this func isShortFlagWithNoDefaultValue is confusing.

Sorry, I don't understand why you feel it is confusing. Does the name not reflect the behaviour of the function?

change these helper funcs to return (bool, error)

What would be the error case?

check the flags before hand

This is the code which checks flag, I don't understand what we could do be fore this.

A lot of the if flag == nil { and if len(name) == 0 { code could be removed making this code clearer.

if len(name) == 0 can be removed I think, ys. flag == nil is still important. It's what tells us if this is a flag or an arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would rather I omit the refactoring, and only add the test case for now, I'm fine with that.

return strings.HasPrefix(arg, "-") && !strings.Contains(arg, "=") && len(arg) == 2 && !shortFlagHasDefaultValue(arg[1:], flags)
}

func shortFlagHasDefaultValue(name string, fs *flag.FlagSet) bool {
if len(name) == 0 {
return false
}

flag := fs.ShorthandLookup(name[:1])
if flag == nil {
return false
}
return flag.NoOptDefVal != ""
}

// argsMinusFirstX removes only the first x from args. Otherwise, commands that look like
// openshift admin policy add-role-to-user admin my-user, lose the admin argument (arg[4]).
func argsMinusFirstX(args []string, x string) []string {
Expand Down Expand Up @@ -576,16 +577,17 @@ func (c *Command) Traverse(args []string) (*Command, []string, error) {
flags := []string{}
inFlag := false

c.mergePersistentFlags()

for i, arg := range args {
switch {
// A long flag with a space separated value
case strings.HasPrefix(arg, "--") && !strings.Contains(arg, "="):
// TODO: this isn't quite right, we should really check ahead for 'true' or 'false'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this TODO because it's actually not an issue. The current behaviour is correct and matches stripFlags().

inFlag = !hasNoOptDefVal(arg[2:], c.Flags())
inFlag = !hasDefaultValue(arg[2:], c.Flags())
flags = append(flags, arg)
continue
// A short flag with a space separated value
case strings.HasPrefix(arg, "-") && !strings.Contains(arg, "=") && len(arg) == 2 && !shortHasNoOptDefVal(arg[1:], c.Flags()):
case isShortFlagWithNoDefaultValue(arg, c.Flags()):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may well be wrong, but my first glance makes me believe this may not be right. For short flags one could do (pseudo code)

BoolFlagP("bool1", "a")
BoolFlagP("bool2", "b")
StringFlagP("string", "s")

and on the command line do:
-abs hello

And be perfectly correct. This code will, I think, look only at a will completely ignore b and s and won't handle hello at all. The other code you referenced does seem to better handle short flags.

Copy link
Contributor Author

@dnephin dnephin Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that still needs to be solved, however that's not related to the original issue in #615 , right? The bug was that the flags were not in c.Flags() because they weren't merged yet.

Should we open a new issue for that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does seem to have a number of unrelated problems. I guess the reason it kinda works is because the call the ParseFlags() near the end does the merge. Merging (like in 615) at least seems to give this function a chance to be better.

Can you add a bunch of new _test for Traverse as you try to clean things up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are already a few tests for this function. I can add more as issues are fixed, but I think it would be good to merge this now, since it does fix one of the issues.

I opened #618 to track the other issue you found.

inFlag = true
flags = append(flags, arg)
continue
Expand Down
21 changes: 21 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1552,6 +1552,27 @@ func TestTraverseWithTwoSubcommands(t *testing.T) {
}
}

func TestTraversalWithPersistentFlags(t *testing.T) {
rootCmd := &Command{Use: "root", TraverseChildren: true}
rootCmd.PersistentFlags().BoolP("toggle", "t", false, "Help message for toggle")
subCmd := &Command{
Use: "sub",
DisableFlagParsing: true,
}
rootCmd.AddCommand(subCmd)

c, args, err := rootCmd.Traverse([]string{"-t", "sub", "abc"})
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if len(args) != 1 && args[0] != "abc" {
t.Errorf("Wrong args: %v", args)
}
if c.Name() != subCmd.Name() {
t.Errorf("Expected command: %q, got: %q", subCmd.Name(), c.Name())
}
}

// TestUpdateName checks if c.Name() updates on changed c.Use.
// Related to https://github.com/spf13/cobra/pull/422#discussion_r143918343.
func TestUpdateName(t *testing.T) {
Expand Down