ipfs / go-ipld-format

IPLD Node and Resolver interfaces in Go
https://github.com/ipld/ipld
MIT License
64 stars 26 forks source link

Add a DAG walker with support for IPLD `Node`s #39

Closed schomatis closed 5 years ago

schomatis commented 6 years ago

Imported from https://github.com/ipfs/go-ipfs/pull/5257. Used in https://github.com/ipfs/go-unixfs/pull/12.

This PR creates a DAG walker to abstract the DAG traversal logic from other parts of the code like the DAG reader. The walker itself started very coupled with the IPLD node logic but now works on a more generic NavigableNode interface that would permit to operate on a more general concept of node (e.g., a Gx dependency), so the walker.go file can now be extracted elsewhere (where? new repo?).

schomatis commented 6 years ago

@Stebalien Could you take a look at this please? Particularly the node promise logic that was extracted from the DAG reader.

schomatis commented 5 years ago

@Mr0Grog Could you take a second look at the DAG walker please? This is a spin-off from the initial PR you already reviewed, I have incorporated many of your suggestions and also have split the logic adding a new NavigableNode interface. Also, could you take a closer look at the new documentation please?

schomatis commented 5 years ago

Thanks for reviewing this (again) @Mr0grog !!

schomatis commented 5 years ago

@Stebalien This has low priority but do know that your review is needed to move ahead with this (and apply it in the DAG reader, https://github.com/ipfs/go-unixfs/pull/12).

schomatis commented 5 years ago

With this second round of review I'm pretty satisfied about the documentation and general design (thanks to @Mr0grog) so I'll tag this as blocked pending @Stebalien's review.

schomatis commented 5 years ago

@Stebalien

Stebalien commented 5 years ago

So as not to block this indefinitely, I'm assigning @warpfork (who will be working on the IPLD APIs anyways). I'll trust both of your judgments, merge when you agree on something.

schomatis commented 5 years ago

/cc @warpfork

warpfork commented 5 years ago

I'd be happy with this being merged: LGTM :+1:

I'd also +1 some of @Mr0grog 's comments about being cautious of speculative generality, though. Long run, I'd rather make Really Good traversables for IPLD nodes; and come up with more smooth interfaces to mapping other kinds of data into our existing node interfaces, rather than try to maintain a hypergenericized traversables library with a whole set of parallel node interfaces (in golang, nonetheless, where anything "generic" is already... shall we say "a big ask"). Introducing the interface in this PR makes sense here and now, and I'm :+1: to this code, but I'd be :-1: to trying to generalize it further or extract it into another repo and give it further generic-beyond-ipld life; another repo and another drifting set of interfaces with separate git history would heap on more overhead when trying to keep things developing in the same direction in the future.

schomatis commented 5 years ago

@warpfork So, actually the title is misleading since I haven't updated it to reflect the current code, this is actually a DAG walker, and by DAG I just mean a graph of any kind of node, not just IPLD nodes. I've seen over and over again code repeating itself just do a basic DAG iteration (where a DAG is the known DAG of IPLD nodes, but also a filesystem hierarchy, a Gx dependency graph, etc). The actual IPLD part is encapsulated in the navipld.go file.

That said, if it helps move things forward let's leave all the code here, and evaluate separating the walker.go file in another repo only if it's really worth it, i.e., if we can prove this can actually be used somewhere else besides the file DAG reader in ipfs/go-unixfs#12. I agree with you that we should be cautious about creating new repos "just in case".

schomatis commented 5 years ago

@warpfork Removed the TODO and updated the Cid references, tests are passing now. Would you like to change anything else? If not, could you approve the PR please?

warpfork commented 5 years ago

... I actually thought I hit the approve button yesterday, my bad :)

schomatis commented 5 years ago

Great, thanks for taking the time to review and unblock this!

hannahhoward commented 5 years ago

Woohoo excited to check back in and see this was merged! Yay @schomatis !

Stebalien commented 5 years ago

Yes. Thanks @schomatis for your patience on this and thanks @warpfork for reviewing!