refactor: Refactor commit nodes #892
Conversation
Node no longer really does anything and only serves to misdirect and confuse
As well as being clearer IMO in it's own right, this will make life easier shortly
Is much easier if currentCid stays local. Had to add a couple of checks for when cid is provided as otherwise it starts to take a different (and incorrect) code-path, will likely be removed/optimized shortly.
Also fixes an explain bug where it was incorrectly reporting field as 'C'
A bit of a nitpick, but it is also now located next to where it is used/returned
Doesnt actually do anything anymore
AndrewSisley
added
refactor
It no longer served a purpose
Codecov Report
@@ Coverage Diff @@
## develop #892 +/- ##
===========================================
- Coverage 59.78% 59.68% -0.10%
===========================================
Files 157 156 -1
Lines 17423 17306 -117
===========================================
- Hits 10416 10329 -87
+ Misses 6066 6042 -24
+ Partials 941 935 -6
|
Bit weird the original way, not sure why I did that :)
query/graphql/planner/commit.go
Outdated
| newqueue := make([]*cid.Cid, len(heads)+len(n.queuedCids)) | ||
| copy(newqueue[len(heads):], n.queuedCids) | ||
| n.queuedCids = newqueue |
There was a problem hiding this comment.
suggestion (non-blocking): this can be simplified to
n.queuedCids = append(make([]*cid.Cid, len(heads)), n.queuedCids)There was a problem hiding this comment.
the suggestion looks incorrect to me - that would result in [a, b, c, nil, nil] instead of the (desired) [nil, nil, a, b, c] no?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ahhh I see I misread - cheers!
- do this
| if len(n.queuedCids) > 0 { | ||
| currentCid = n.queuedCids[0] | ||
| n.queuedCids = n.queuedCids[1:(len(n.queuedCids))] | ||
| } else if n.parsed.Cid.HasValue() && n.parsed.DocKey == "" { |
There was a problem hiding this comment.
praise: I remember discussing the use of the parsed values instead of the duplicated fields and I'm happy to see that you made the change :)
There was a problem hiding this comment.
Cheers, was a good suggestion and cuts a lot of the weirdness/effort out regarding some of the conversions.
Cheers! Will try leave it for a few more hours, but with most people off/otherwise-occupied atm I'm not sure it will get anymore attention. (is also just a refactoring with reasonable and growing test coverage) |
* Remove headsetScanNode Node no longer really does anything and only serves to misdirect and confuse * Make cid an option-type As well as being clearer IMO in it's own right, this will make life easier shortly * Use parsed.Cid over duplicate prop * Dont store iteration state Is much easier if currentCid stays local. Had to add a couple of checks for when cid is provided as otherwise it starts to take a different (and incorrect) code-path, will likely be removed/optimized shortly. * Remove legacy comment * Use parsed.Depth over duplicate prop * Use parsed.DocKey over duplicate prop * Use parsed.Field over duplicate prop Also fixes an explain bug where it was incorrectly reporting field as 'C' * Simplify commit node constructor * Set currentValue only when valid A bit of a nitpick, but it is also now located next to where it is used/returned * Replace legacy comment with correct comment * Add cid related comment * Remove type-unsafe list with slice magic * Remove odd comment * Remove commitSelectNode Doesnt actually do anything anymore * Remove commitSelectTopNode It no longer served a purpose
Relevant issue(s)
Resolves #858
Description
Refactors the commit nodes, reducing their number from 4, to 1.
Strongly recommend reviewing commit by commit, as there is quite a lot going on in this PR and I hope it is a bit easier to see the functional equivalence at commit-level, as well as the reasons for the change.
Was tempted to rename
dagScantocommitNodeor similar now, but laziness won out - let me know your thoughts!