[WIP] add missing dag.tree() #819
Conversation
* add method `tree` in dag. But it is not following the links when options.recursive is true at this moment. * add some tests for dag operations (put, get, and tree).
There was a problem hiding this comment.
Thanks for the PR. The general idea is correct. It works the same way as the other DAG command put and get currently do.
If this PR is cleaned up, it can be merged even without supporting recursion.
Though a proper fix (that you couldn't know of) would be to use https://github.com/ipld/js-ipld. There is already a PR open for DAG GET (#755).
When using js-ipld, a lot of things will change, so there's no point of adding recursion for now.
src/dag/tree.js
Outdated
| block(send).get(cid, options, (err, ipfsBlock) => { | ||
| if (err) return callback(err, null) | ||
|
|
||
| let codec = ipfsBlock.cid.codec |
There was a problem hiding this comment.
Nit-pick: This could be a const.
| const f = require('./utils/factory') | ||
|
|
||
| describe('.dag', function () { | ||
| this.timeout(50 * 1000) // slow CI |
There was a problem hiding this comment.
I guess that was copy&pasted. Generally try to stick to the default timeouts and see if they are long enough,
There was a problem hiding this comment.
I tried with the default timeout but it was not enough.
| }) | ||
| }) | ||
|
|
||
| it('.dag.put', (done) => { |
There was a problem hiding this comment.
Looks like a left-over from copy&pasting. Please remove it if that's the case.
There was a problem hiding this comment.
I did not know how to name this test. I looked into log.spec.js and repo.spec.js and tried to follow the convention there.
There was a problem hiding this comment.
I was just wondering as this test doesn't have a call to tree().
There was a problem hiding this comment.
I'm sorry, my review was sloppy. I thought I was still looking at the file for the tree() tests. I missed that you've added another test file for testing put() and get(). Thanks for that.
Next time this should be a separate commit, as it is unrelated to adding the dag.tree(). Commits should be self contained. The reason is that it makes it easier if you look through the Git history, but also if you e.g. need to revert a change. If we would need to revert the commit where dag.tree() was added, suddenly unrelated tests would be removed as well.
| }) | ||
| }) | ||
|
|
||
| it('.dag.get', (done) => { |
There was a problem hiding this comment.
Looks like a left-over from copy&pasting. Please remove it if that's the case.
src/dag/tree.js
Outdated
|
|
||
| // FIXME: handle case when options.recursive is true | ||
| block(send).get(cid, options, (err, ipfsBlock) => { | ||
| if (err) return callback(err, null) |
There was a problem hiding this comment.
In the error callbacks, the return value is normally undefined and not null. So this would then just be return callback(err).
|
Please have a look at the failures of the continuous integration. The code coverage decreased. It gives some indication on which test cases should be added. |
Hi!
I was trying to solve #790 but I decided to stop here because I'm not sure if this is the right path and I decide to ask you what to do next.
Initially, I searched in the HTTP-API for an endpoint "/dag/tree", but I did not find it. I also ran the go daemon and made some tests but it seems that the endpoint is not there yet. According to the documentation the method
dag.treeis only implemented in js-ipfs.After that, I decided to write some code that is able to get the paths (no recursion yet) in order to get some exposure to the ipfs ecosystem and try to understand some of its components. I also added some tests for that.
Should I continue in this direction and add the logic for the recursive following of the links or the solution is finishing the HTTP-API?
Thank you! (: