-
Notifications
You must be signed in to change notification settings - Fork 296
chore: update method signatures in line with ipfs/interface-ipfs-core#277 #786
Conversation
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.
SGTM
Left just a note for a small improvement.
src/files/mv.js
Outdated
sources = args[0] | ||
} else { | ||
// support ipfs.file.mv(src, dest, opts, cb) and ipfs.file.cp(src1, src2, dest, opts, cb) | ||
sources = args | ||
} |
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.
Maybe we can have the above logic abstracted in a util function since the code is replicated in mv
and cp
. What do you think @achingbrain ?
536a01e
to
48ebaea
Compare
48ebaea
to
f959aad
Compare
@vasco-santos I've updated the PR and implemented your suggestion but I'm having problems getting Jenkins to pass. Lots of instability around the ping tests. They seem to be failing for master on Jenkins too, any ideas? |
Thanks @achingbrain ! I checked the CI log and it was a timeout in windows environment. So, I started the CI again to check if it gets green again. As your changes are not related to the test failing, and master is red in the same tests, I think this PR is ok. |
Ha, I've been restarting it all morning. We should probably quarantine flaky tests until we can make them stable. |
Yeah, I agree that it is the best solution here @achingbrain ! |
Great, can this be merged & released? |
...and could there please also be a corresponding release of Releasing v1 of this modules would make this all so much easier! |
For me, it is good to go and be released. I can handle the |
I think everyone's on holiday.. I'll merge this and hopefully someone can do the do when people are back. |
See ipfs-inactive/interface-js-ipfs-core#277 for more information