-
Notifications
You must be signed in to change notification settings - Fork 42
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
forward the remaining of the stdin args to the server #83
Conversation
This should probably have some tests... any other feedback? |
6240f2b
to
a1bb029
Compare
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
=========================================
+ Coverage 43.02% 43.6% +0.57%
=========================================
Files 23 24 +1
Lines 1922 1961 +39
=========================================
+ Hits 827 855 +28
- Misses 960 970 +10
- Partials 135 136 +1
Continue to review full report at Codecov.
|
@@ -0,0 +1,66 @@ | |||
package cmds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be able to continue treating the argument reader as an io.ReadCloser
so we can't use bufio.Scanner
.
cli/parse_test.go
Outdated
} else if err != nil { | ||
t.Fatal(err) | ||
} | ||
bodyArgs = append(bodyArgs, next) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface is a bit awkward but I'm not a fan of the scanner interface either.
// However, such is life. | ||
fileReader = files.NewMultiFileReader(files.NewSliceFile("", "", []files.File{ | ||
files.NewReaderFile("stdin", "", bodyArgs, nil), | ||
}), true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah....
(for the record, I hate this PR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer the scanner interface, but other than that, this LGTM.
A couple tests would be nice just so we don't have to wait until it gets bubbled up into ipfs to see any regressions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGWM, only thing I don't like (but it is old) is that we send body args with \n
as separator.
Note: I've already bubbled it to test and it seems to be working (the one issue is a slight change in an error message). |
I have little preference so I'll switch back to it. |
ec4e83f
to
49a26f3
Compare
Apparently, it's not thread-safe.
f8697b6
to
9160bdc
Compare
No description provided.