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

short opt handling: fix parsing #758

Merged
merged 2 commits into from
Aug 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 52 additions & 5 deletions command.go
Expand Up @@ -181,16 +181,49 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
return set, set.Parse(append([]string{"--"}, args...))
}

if c.UseShortOptionHandling {
args = translateShortOptions(args)
}

if !c.SkipArgReorder {
args = reorderArgs(args)
}

PARSE:
err = set.Parse(args)
if err != nil {
if c.UseShortOptionHandling {
// To enable short-option handling (e.g., "-it" vs "-i -t")
// we have to iteratively catch parsing errors. This way
Copy link
Member

Choose a reason for hiding this comment

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

Don't use 2 spaces after a period, this is not a typewriter ;)
https://www.cultofpedagogy.com/two-spaces-after-period/

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 am much older than I look ;-)

// we achieve LR parsing without transforming any arguments.
// Otherwise, there is no way we can discriminate combined
// short options from common arguments that should be left
// untouched.
errStr := err.Error()
trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: ")
if errStr == trimmed {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dirty, why can't we return an error of a special type or ParseResult that does not rely on string matching?

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 agree, but currently, there is no other way around that as the standard lib doesn't provide a special error type for that yet. I opened an issue for golang about that which is slowly taking shape (see golang/go#26822) but that will take a longer while until all details are figured out.

In other words, I don't think there is any other way around that.

return nil, err
}
// regenerate the initial args with the split short opts
newArgs := Args{}
for i, arg := range args {
if arg != trimmed {
newArgs = append(newArgs, arg)
continue
}
shortOpts := translateShortOptions(set, Args{trimmed})
if len(shortOpts) == 1 {
return nil, err
}
// add each short option and all remaining arguments
newArgs = append(newArgs, shortOpts...)
newArgs = append(newArgs, args[i+1:]...)
args = newArgs
// now reset the flagset parse again
set, err = flagSet(c.Name, c.Flags)
if err != nil {
return nil, err
}
set.SetOutput(ioutil.Discard)
goto PARSE
}
}
return nil, err
}

Expand Down Expand Up @@ -232,11 +265,25 @@ func reorderArgs(args []string) []string {
return append(flags, nonflags...)
}

func translateShortOptions(flagArgs Args) []string {
func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string {
allCharsFlags := func (s string) bool {
for i := range s {
f := set.Lookup(string(s[i]))
if f == nil {
return false
}
}
return true
}

// separate combined flags
var flagArgsSeparated []string
for _, flagArg := range flagArgs {
if strings.HasPrefix(flagArg, "-") && strings.HasPrefix(flagArg, "--") == false && len(flagArg) > 2 {
if !allCharsFlags(flagArg[1:]) {
flagArgsSeparated = append(flagArgsSeparated, flagArg)
continue
}
for _, flagChar := range flagArg[1:] {
flagArgsSeparated = append(flagArgsSeparated, "-"+string(flagChar))
}
Expand Down
48 changes: 46 additions & 2 deletions command_test.go
Expand Up @@ -57,6 +57,52 @@ func TestCommandFlagParsing(t *testing.T) {
}
}

func TestParseAndRunShortOpts(t *testing.T) {
cases := []struct {
testArgs []string
expectedErr error
expectedArgs []string
}{
{[]string{"foo", "test", "-a"}, nil, []string{}},
{[]string{"foo", "test", "-c", "arg1", "arg2"}, nil, []string{"arg1", "arg2"}},
{[]string{"foo", "test", "-f"}, nil, []string{}},
{[]string{"foo", "test", "-ac", "--fgh"}, nil, []string{}},
{[]string{"foo", "test", "-af"}, nil, []string{}},
{[]string{"foo", "test", "-cf"}, nil, []string{}},
{[]string{"foo", "test", "-acf"}, nil, []string{}},
{[]string{"foo", "test", "-invalid"}, errors.New("flag provided but not defined: -invalid"), []string{}},
{[]string{"foo", "test", "-acf", "arg1", "-invalid"}, nil, []string{"arg1" ,"-invalid"}},
}

var args []string
cmd := Command{
Name: "test",
Usage: "this is for testing",
Description: "testing",
Action: func(c *Context) error {
args = c.Args()
return nil
},
SkipArgReorder: true,
UseShortOptionHandling: true,
Flags: []Flag{
BoolFlag{Name: "abc, a"},
BoolFlag{Name: "cde, c"},
BoolFlag{Name: "fgh, f"},
},
}

for _, c := range cases {
app := NewApp()
app.Commands = []Command{cmd}

err := app.Run(c.testArgs)

expect(t, err, c.expectedErr)
expect(t, args, c.expectedArgs)
}
}

func TestCommand_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) {
app := NewApp()
app.Commands = []Command{
Expand Down Expand Up @@ -293,7 +339,6 @@ func TestCommandSkipFlagParsing(t *testing.T) {
}

for _, c := range cases {
value := ""
args := []string{}
app := &App{
Commands: []Command{
Expand All @@ -305,7 +350,6 @@ func TestCommandSkipFlagParsing(t *testing.T) {
},
Action: func(c *Context) {
fmt.Printf("%+v\n", c.String("flag"))
value = c.String("flag")
args = c.Args()
},
},
Expand Down