Share more code between Traverse and stripFlags #617
Conversation
Signed-off-by: Daniel Nephin <[email protected]>
dnephin
force-pushed
the
share-more-code-with-traverse
branch
from
7558591 to
46f41fc
Compare
| 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' |
There was a problem hiding this comment.
I removed this TODO because it's actually not an issue. The current behaviour is correct and matches stripFlags().
| 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()): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| return flag.NoOptDefVal != "" | ||
| } | ||
|
|
||
| func isShortFlagWithNoDefaultValue(arg string, flags *flag.FlagSet) bool { |
There was a problem hiding this comment.
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
- change these helper funcs to return
(bool, error), or - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If you would rather I omit the refactoring, and only add the test case for now, I'm fine with that.
|
Pls ignore my previous comment.
…On Thursday, 18 January 2018, Daniel Nephin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In command.go
<#617 (comment)>:
> @@ -491,6 +468,30 @@ Loop:
return commands
}
+func hasDefaultValue(name string, fs *flag.FlagSet) bool {
+ flag := fs.Lookup(name)
+ if flag == nil {
+ return false
+ }
+ return flag.NoOptDefVal != ""
+}
+
+func isShortFlagWithNoDefaultValue(arg string, flags *flag.FlagSet) bool {
If you would rather I omit the refactoring, and only add the test case for
now, I'm fine with that.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#617 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAu9Mr3Hg0mkEw4wXE27zq41D-4-DXHmks5tLn5SgaJpZM4Rhztc>
.
|
| @@ -491,6 +468,30 @@ Loop: | |||
| return commands | |||
| } | |||
|
|
|||
| func hasDefaultValue(name string, fs *flag.FlagSet) bool { | |||
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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.
|
@dnephin looks like this needs a rebase 😅 |
|
This PR is being marked as stale due to a long period of inactivity |
|
+label:needs investigation It seems that this PR is worth merging in. It has a test and seems the work needed to be done here is just fixing conflicts. @dnephin Any interest in fixing the conflict? We can try and merge it in soon after if still applicable. WDYT? |
Includes, and closes #616