-
Notifications
You must be signed in to change notification settings - Fork 79
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
refactor: rename share to chunk #299
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #299 +/- ##
==========================================
+ Coverage 80.89% 82.24% +1.34%
==========================================
Files 8 7 -1
Lines 869 614 -255
==========================================
- Hits 703 505 -198
+ Misses 119 66 -53
+ Partials 47 43 -4 ☔ View full report in Codecov by Sentry. |
What about celestia-node, celestia-app, and other repos? |
celestia-node has ~183 occurrences of share and ~4 for chunk. celestia-app almost always uses shares and not chunk. I had a preference for share but you preferred chunk. See #262 (comment). |
If share is overwhelmingly used more than chunk then I'm not against using share |
But in this repo it seems that chunk is used more than share? |
This repo is split. 188 occurrences of chunk and 300 occurrences of share. I still lean slightly towards share because usage in downstream repos but not strongly opinionated on it. Can do whichever reviewers prefer. |
Wonder what Celestia node API uses. I also do think it seems people say
share more than chunks
…On Wed, 21 Feb 2024, 22:47 Rootul P, ***@***.***> wrote:
This repo is split. 188 occurrences of chunk and 300 occurrences of share.
I still lean slightly towards share because usage in downstream repos but
not strongly opinionated on it. Can do whichever reviewers prefer.
—
Reply to this email directly, view it on GitHub
<#299 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGOEBP3FTKU6GUANXC47SDYUZTNNAVCNFSM6AAAAABDTZA5W6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGAZDOOBZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
celestia-node API uses share: https://node-rpc-docs.celestia.org/?version=v0.13.0#share. I don't see instances of chunk in those docs. |
If most apis and docs use share I'd guess it makes more sense to use share
in the code
…On Wed, 21 Feb 2024, 22:51 Rootul P, ***@***.***> wrote:
celestia-node API uses share:
https://node-rpc-docs.celestia.org/?version=v0.13.0#share. I don't see
instances of chunk in those docs.
—
Reply to this email directly, view it on GitHub
<#299 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGOEBICMEB3UUX7SRH6UQ3YUZT5HAVCNFSM6AAAAABDTZA5W6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJYGAZTSOBWGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Agreed, can do. |
Closes #215
I opted to use chunk instead of share because @musalbas preferred it and chunk already appears in the exported API of this library (e.g.
ValidateChunkSize
,MaxChunks
).Technically
Shares
is in the public API too (it's a field ofErrByzantineData
) but this PR doesn't introduce any breaking changes b/c IMO it's not worth breaking API over this name change.