Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Fix #140: ipfs add url wrap doesn't work #750

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Fix #140: ipfs add url wrap doesn't work #750

merged 2 commits into from
Apr 26, 2018

Conversation

hacdias
Copy link
Contributor

@hacdias hacdias commented Apr 25, 2018

ipfs.util.addFromURL now works well with the wrapWithDirectory option.

@ghost ghost assigned hacdias Apr 25, 2018
@ghost ghost added the in progress label Apr 25, 2018
@hacdias hacdias requested review from vmx and daviddias April 25, 2018 15:26
Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

The change looks good. Could you please add a test case for it?

Also I'm a fan of good commit messages, so that one can find out why something was done without looking into GitHub. Some good introduction on that topic can be found at https://chris.beams.io/posts/git-commit/

@hacdias
Copy link
Contributor Author

hacdias commented Apr 25, 2018

@vmx done! Yeah, I just didn't care much because I'd squash everything before merging and give a good description. But it's fixed now anyway.

Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

The change itself looks good. But it seems that using an ipfs.io as URL makes the test time out.

@vmx
Copy link
Contributor

vmx commented Apr 25, 2018

In regards to the commit message: I normally put the "good" commit message on the first commit, even if things get squashed. This way also the commit message can be reviewed.

@hacdias
Copy link
Contributor Author

hacdias commented Apr 25, 2018

@vmx it was probably timing out because I forgot the done() in the end. Let's see what happens now.

Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

The change looks good. The CI is flaky, but it doesn't seem to be related to this change.

@hacdias
Copy link
Contributor Author

hacdias commented Apr 26, 2018

Thanks @vmx. Ping @diasdavid

@daviddias daviddias merged commit f6f1bf0 into master Apr 26, 2018
@daviddias daviddias deleted the fix/140 branch April 26, 2018 10:58
@ghost ghost removed the in progress label Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants