search

filecoin-project / go-amt-ipld

Implementation of an array mapped trie using go and ipld
Other
9 stars 15 forks source link

polish(diff): add diff method accepting options for diffing AMTs with nondefault options #49

Closed frrist closed 3 years ago

frrist commented 3 years ago
frrist commented 3 years ago

@Stebalien you're correct, it doesn't work (added test to make sure). Closing.

frrist commented 3 years ago

reopening as there is value in controlling the options an AMT node is loaded with, e.g. being able to diff 2 AMT nodes that have Bitwidths differing from the default value.

codecov-commenter commented 3 years ago

Codecov Report

Merging #49 (4a5d2da) into master (2045095) will decrease coverage by 0.25%. The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   66.01%   65.75%   -0.26%     
==========================================
  Files           6        6              
  Lines         512      514       +2     
==========================================
  Hits          338      338              
- Misses         92       93       +1     
- Partials       82       83       +1
Impacted Files Coverage Δ
diff.go 66.12% <50.00%> (-0.72%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2045095...4a5d2da. Read the comment docs.

Stebalien commented 3 years ago

reopening as there is value in controlling the options an AMT node is loaded with, e.g. being able to diff 2 AMT nodes that have Bitwidths differing from the default value.

This is valuable, but the current method isn't the right way to go about this as we can still end up diffing AMTs with different bit-widths leading to nonsensical results. We could just check inside Diff, but it's likely easier to just make Diff accept opts ...Option like LoadAMT does, forwarding the option to LoadAMT.