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

fix(add): refer to ipfs issue #5456 #121

Merged
merged 1 commit into from
Sep 19, 2018
Merged

Conversation

overbool
Copy link
Contributor

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

Fixes: ipfs/kubo#5456

@overbool
Copy link
Contributor Author

overbool commented Sep 17, 2018

@keks I'm trying to solve the ipfs/kubo#5456, but i have no idea whether my solution is correct, could you help me review my pr? Thx

@overbool overbool force-pushed the fix/ipfs-#5456 branch 2 times, most recently from 738f586 to 2abaff8 Compare September 17, 2018 16:47
@overbool overbool changed the title [WIP]: fix(add): refer to ipfs issue #5456 fix(add): refer to ipfs issue #5456 Sep 17, 2018
@Stebalien Stebalien requested a review from keks September 17, 2018 20:17
@Stebalien
Copy link
Member

I think this is the correct fix but I'd like @keks to sign off on it.

@Stebalien
Copy link
Member

However, it needs a go fmt.

Copy link
Contributor

@keks keks left a comment

Choose a reason for hiding this comment

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

yes, fmt it and then it should be golden!

License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@overbool
Copy link
Contributor Author

@Stebalien @keks I had fixed it

@keks
Copy link
Contributor

keks commented Sep 18, 2018

This looks good!

@Stebalien do you think there should be a test for this edge case to prevent regressions?

@Stebalien
Copy link
Member

@keks yes. However, I like merging bug fixes, with or without tests, as long as we leave the bug open.

@Stebalien Stebalien merged commit d954e86 into ipfs:master Sep 19, 2018
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