Refactor trickle DAG builder #4730
Conversation
schomatis
changed the title
License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>
schomatis
changed the title
|
@whyrusleeping Could you review this please? |
| for j := 0; j < layerRepeat && !db.Done(); j++ { | ||
| next := db.NewUnixfsNode() | ||
| if err := fillTrickleRec(db, next, i); err != nil { | ||
| for depth := 1; ; depth++ { |
There was a problem hiding this comment.
Code nit: You can write this as for depth := 1; depth < maxDepth || maxDepth < 0; depth++ {.
There was a problem hiding this comment.
I put it separately to highlight the fact that maxDepth is an optional parameter (that will most commonly be unset, which is the base case) but I can move it all together to make it more compact if you think it's more in line with Go coding principles.
There was a problem hiding this comment.
Sounds reasonable. I don't feel strongly either way.
| for layer := 0; layer < layerRepeat; layer++ { | ||
| if db.Done() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
To avoid repeating the same condition in both fors, and make it more clear that if db.Done is true nothing more needs to be done in this function and it should just exit there (removing the final return from the code).
|
@Stebalien Thanks for the review! |
|
Thanks @schomatis, and congrats on your first PR landing :) |
Refactoring of the logic of the trickle DAG builder to simplify the future patch that will add support for Filestore (#4052).
The
Layoutfunction is mirroring the logic infillTrickleRecduplicating almost entirely its code. The refactoredfillTrickleRecnow absorbs both scenarios of building a trickle tree with or without a maximum depth (Layoutdidn't have a maximum depth, onlyfillTrickleRecdid). This is accomplished abusing thedepthparameter (now renamed asmaxDepth) to make it optional (a negative value is interpreted as no maximum). This hack is due to the fact that Go doesn't support optional parameters, suggestions are welcome for a more elegant solution.I'm submitting this WIP PR to have feedback on this refactoring, as it will be useful to extend it to other functions (
Append,appendFillLastChild,appendRec) which also replicate similar loop patterns of the sort:https://github.com/ipfs/go-ipfs/blob/cc01b7f188622e7148ce041b9d09252c85041d9f/importer/trickle/trickledag.go#L212-L224