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

Improve stdin parsing #1238

Merged
merged 7 commits into from
May 18, 2015
Merged

Improve stdin parsing #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff9cb9e
parse_test: test multiline stdin
chriscool May 17, 2015
97ab64a
parse: don't use stdin if there are arguments
chriscool May 17, 2015
5503c4c
parse_test: improve error messages
chriscool May 17, 2015
942e964
parse: fix parsing stdin args
chriscool May 17, 2015
c93d583
parse_test: add tests with 2 args and stdin
chriscool May 17, 2015
53a80e2
parse: fix arg number check
chriscool May 17, 2015
275ec7c
parse_test: test stdin enabled and not variadic arg
chriscool May 17, 2015
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
35 changes: 22 additions & 13 deletions commands/cli/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
// if we have more arg values provided than argument definitions,
// and the last arg definition is not variadic (or there are no definitions), return an error
notVariadic := len(argDefs) == 0 || !argDefs[len(argDefs)-1].Variadic
if notVariadic && numInputs > len(argDefs) {
return nil, nil, fmt.Errorf("Expected %v arguments, got %v: %v", len(argDefs), numInputs, inputs)
if notVariadic && len(inputs) > len(argDefs) {
return nil, nil, fmt.Errorf("Expected %v arguments, got %v: %v", len(argDefs), len(inputs), inputs)
}

stringArgs := make([]string, 0, numInputs)
Expand All @@ -250,29 +250,38 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi

var err error
if argDef.Type == cmds.ArgString {
if stdin == nil {
if stdin == nil || !argDef.SupportsStdin {
// add string values
stringArgs, inputs = appendString(stringArgs, inputs)

} else if argDef.SupportsStdin {
// if we have a stdin, read it in and use the data as a string value
stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
if err != nil {
return nil, nil, err
} else {
if len(inputs) > 0 {
// don't use stdin if we have inputs
stdin = nil
} else {
// if we have a stdin, read it in and use the data as a string value
stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
if err != nil {
return nil, nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

im not sure this works with all commands-- do we have a case where we use both arguments AND stdin? maybe some of the ipfs object put type things? (although those commands are not very intuitive right now, so perhaps they can change)

Copy link
Member

Choose a reason for hiding this comment

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

in general, this part of the cmdslib is a bit of a mess. ideas to clean it up would be great.

}
}

} else if argDef.Type == cmds.ArgFile {
if stdin == nil {
if stdin == nil || !argDef.SupportsStdin {
// treat stringArg values as file paths
fileArgs, inputs, err = appendFile(fileArgs, inputs, argDef, recursive)
if err != nil {
return nil, nil, err
}

} else if argDef.SupportsStdin {
// if we have a stdin, create a file from it
fileArgs, stdin = appendStdinAsFile(fileArgs, stdin)
} else {
if len(inputs) > 0 {
// don't use stdin if we have inputs
stdin = nil
} else {
// if we have a stdin, create a file from it
fileArgs, stdin = appendStdinAsFile(fileArgs, stdin)
}
}
}

Expand Down
54 changes: 49 additions & 5 deletions commands/cli/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,23 @@ func TestArgumentParsing(t *testing.T) {
commands.StringArg("a", true, true, "some arg").EnableStdin(),
},
},
"stdinenabled2args": &commands.Command{
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg"),
commands.StringArg("b", true, true, "another arg").EnableStdin(),
},
},
"stdinenablednotvariadic": &commands.Command{
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg").EnableStdin(),
},
},
"stdinenablednotvariadic2args": &commands.Command{
Arguments: []commands.Argument{
commands.StringArg("a", true, false, "some arg"),
commands.StringArg("b", true, false, "another arg").EnableStdin(),
},
},
},
}

Expand All @@ -166,10 +183,10 @@ func TestArgumentParsing(t *testing.T) {
}
req, _, _, err := Parse(cmd, f, rootCmd)
if err != nil {
t.Errorf("Command '%v' should have passed parsing", cmd)
t.Errorf("Command '%v' should have passed parsing: %v", cmd, err)
}
if !sameWords(req.Arguments(), res) {
t.Errorf("Arguments parsed from '%v' are not '%v'", cmd, res)
t.Errorf("Arguments parsed from '%v' are '%v' instead of '%v'", cmd, req.Arguments(), res)
}
}

Expand Down Expand Up @@ -220,8 +237,35 @@ func TestArgumentParsing(t *testing.T) {
test([]string{"stdinenabled", "value1", "value2"}, nil, []string{"value1", "value2"})

fstdin := fileToSimulateStdin(t, "stdin1")

test([]string{"stdinenabled"}, fstdin, []string{"stdin1"})
test([]string{"stdinenabled", "value1"}, fstdin, []string{"stdin1", "value1"})
test([]string{"stdinenabled", "value1", "value2"}, fstdin, []string{"stdin1", "value1", "value2"})
test([]string{"stdinenabled", "value1"}, fstdin, []string{"value1"})
test([]string{"stdinenabled", "value1", "value2"}, fstdin, []string{"value1", "value2"})

fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2"})

fstdin = fileToSimulateStdin(t, "stdin1\nstdin2\nstdin3")
test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2", "stdin3"})

test([]string{"stdinenabled2args", "value1", "value2"}, nil, []string{"value1", "value2"})

fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1"})
test([]string{"stdinenabled2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
test([]string{"stdinenabled2args", "value1", "value2", "value3"}, fstdin, []string{"value1", "value2", "value3"})

fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1", "stdin2"})

test([]string{"stdinenablednotvariadic", "value1"}, nil, []string{"value1"})

fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenablednotvariadic"}, fstdin, []string{"stdin1"})
test([]string{"stdinenablednotvariadic", "value1"}, fstdin, []string{"value1"})

test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, nil, []string{"value1", "value2"})

fstdin = fileToSimulateStdin(t, "stdin1")
test([]string{"stdinenablednotvariadic2args", "value1"}, fstdin, []string{"value1", "stdin1"})
test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
}