commands: remove EnableStdin support for StringArg [v2] #2902
Conversation
| @@ -325,6 +301,10 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi | |||
| var file files.File | |||
| var err error | |||
| if fpath == "-" { | |||
| stdInfo, _ := stdin.Stat() | |||
There was a problem hiding this comment.
I know that this error shouldn't happen in practice but we should check for it either way.
Log it under log.Error with some "This shouldn't happen" message.
|
@thomas-gardner great work on that, I left a comment and thing left to do is changing sharness tests to accommodate this change. Also I am writing down why we decided to make this change and what we are changing. |
Kubuxu
added
status/in-progress
|
In this PR we are removing support of direct stdin handling for non-file type arguments in go-ipfs. There are two main reasons why we decided for it:
Those are two main reasons for removal of direct stdin handling for non-file type arguments in |
|
As I am really co-creator of this issue/PR @whyrusleeping you would have to look at it. |
|
|
||
| if _, err := io.WriteString(fstdin, content); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return fstdin | ||
| } | ||
|
|
||
| test([]string{"stdinenabled", "value1", "value2"}, nil, []string{"value1", "value2"}) |
There was a problem hiding this comment.
Are you sure that all these tests need to be removed?
There was a problem hiding this comment.
@whyrusleeping Presently yes, or they'll need to be changed. If you think it's dire, I can add a test funciton to read through the SilceFile and check filenames, which should allow for some less trivial File/String/stdin/error combos.
There was a problem hiding this comment.
We definitely want to have solid coverage over this. I'm not in favor of mass removing tests like this
There was a problem hiding this comment.
(i noticed you added a bunch in now)
With verbose flag: * remove EnableStdin() flags on all StringArg, * remove all unneeded parsing code for StringArg, and print an * informative message if `ipfs` begins reading from a CharDevice, * remove broken go tests for EnableStdin cli parsing, and add some * trivial test cases for reading FileArg from stdin, * add a panic to prevent EnableStdin from being set on * StringArg in the future. Resolves: #2877, #2870 License: MIT Signed-off-by: Thomas Gardner <[email protected]>
License: MIT Signed-off-by: Thomas Gardner <[email protected]>
License: MIT Signed-off-by: Thomas Gardner <[email protected]>
|
LGTM |
| test_expect_success "ipfs cat accept hash from stdin" ' | ||
| echo "$HASH" | ipfs cat >actual | ||
| test_expect_success "ipfs cat accept hash from built input" ' | ||
| echo "$HASH" | xargs ipfs cat >actual |
There was a problem hiding this comment.
I thought this had been reverted, @whyrusleeping @Kubuxu -- the tests on master show it's still here.
There was a problem hiding this comment.
I want this test file have 0 (or minimal) changes in rc4.
With verbose flag:
informative message if
ipfsbegins reading from a CharDevice,trivial test cases for reading FileArg from stdin,
StringArg in the future.
Resolves: #2877, #2870, #2868
License: MIT
Signed-off-by: Thomas Gardner [email protected]