-
Notifications
You must be signed in to change notification settings - Fork 277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: return bytesOfPadding
#810
Conversation
Spun out of celestiaorg#781
@@ -176,19 +176,24 @@ func Test_zeroPadIfNecessary(t *testing.T) { | |||
width int | |||
} | |||
tests := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for reviewers] I found it a little confusing that the zeroPadIfNecessary
function is defined in parse_sparse_shares.go
but tested in shares_test.go
when I would've expected it to be tested in parse_sparse_shares_test.go
.
Additionally this function is used by sparse shares so proposal to move the function to utils.go
(and this test to utils_test.go
)
will do this as a follow-up if reviewers agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense. my guess was this was just an oversight when moving things around the past few weeks
Codecov Report
@@ Coverage Diff @@
## main #810 +/- ##
=======================================
Coverage 23.29% 23.29%
=======================================
Files 71 71
Lines 8790 8790
=======================================
Hits 2048 2048
Misses 6570 6570
Partials 172 172
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable. @rootulp perhaps we can add more info to this PR in a comment or the description as to why this is needed for future reference (and my own understanding lol)
Sorry for not including a proper PR description initially. I updated the PR description above to provide more context. |
Spun out of celestiaorg#781 In celestiaorg#781 , there is a need to calculate the total data length written to a compact share splitter so that a varint of this data length can be written to the first share. It becomes easier to perform this calculation if the utility function `zeroPadIfNecessary` returns the `bytesOfPadding` because the export command can subtract the `bytesOfPadding` from the number of bytes used by all shares in the compact share splitter. Without this change, it may be possible to calculate the # of bytes written by summing the # of bytes written to shares and the number of bytes in the pending share _prior_ to adding padding to the pending share but this option includes more edge cases.
Spun out of #781
In #781 , there is a need to calculate the total data length written to a compact share splitter so that a varint of this data length can be written to the first share. It becomes easier to perform this calculation if the utility function
zeroPadIfNecessary
returns thebytesOfPadding
because the export command can subtract thebytesOfPadding
from the number of bytes used by all shares in the compact share splitter.Without this change, it may be possible to calculate the # of bytes written by summing the # of bytes written to shares and the number of bytes in the pending share prior to adding padding to the pending share but this approach includes more edge cases.