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

test(commands): fix EOF failure with Multipart.Read #3804

Merged
merged 1 commit into from
Mar 21, 2017
Merged

test(commands): fix EOF failure with Multipart.Read #3804

merged 1 commit into from
Mar 21, 2017

Conversation

hoenirvili
Copy link
Contributor

The fix contains the changing nil with io.EOF
because acording to the Read interface implementation
An instance, (in our case Part.Read) of this general case
is that a Reader returning a non-zero number of bytes at
the end of the input stream may return either err == EOF or err == nil.
So this means that the Part.Reader chose to return
io.EOF instead of nil.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 21, 2017

Could you make commit message less verbose: for example: test(commands): fix EOF failure with Multipart.Read.

Also could you add parenthesis around the error logic or split it into separate if condition? It would make this code a bit more readable.

@hoenirvili
Copy link
Contributor Author

@Kubuxu Can you restart just the travis build? I stopped for no reason.

@hoenirvili
Copy link
Contributor Author

@Kubuxu merge?

@whyrusleeping whyrusleeping changed the title This patch fixes fail test TestMultiPart under go 1.8. test(commands): fix EOF failure with Multipart.Read Mar 21, 2017
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM, Thanks for the fix

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.8 milestone Mar 21, 2017
@whyrusleeping whyrusleeping merged commit d065593 into ipfs:master Mar 21, 2017
@hoenirvili hoenirvili deleted the multipart-fix branch March 21, 2017 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants