Skip to content
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: chunk -> share #306

Merged
merged 4 commits into from
Mar 11, 2024
Merged

refactor: chunk -> share #306

merged 4 commits into from
Mar 11, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Mar 5, 2024

Closes #215

@rootulp rootulp requested a review from evan-forbes March 5, 2024 21:52
@rootulp rootulp self-assigned this Mar 5, 2024
@rootulp rootulp requested a review from staheri14 March 6, 2024 15:24
@rootulp rootulp marked this pull request as ready for review March 6, 2024 15:24
codecs.go Show resolved Hide resolved
datasquare.go Show resolved Hide resolved
staheri14
staheri14 previously approved these changes Mar 6, 2024
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestions are optional, the PR as it is LGTM!

@rootulp rootulp marked this pull request as draft March 8, 2024 18:31
@rootulp
Copy link
Collaborator Author

rootulp commented Mar 8, 2024

@rootulp rootulp marked this pull request as ready for review March 8, 2024 18:34
@rootulp rootulp requested a review from staheri14 March 8, 2024 18:34
@@ -109,18 +111,18 @@ func ImportExtendedDataSquare(
// NewExtendedDataSquare returns a new extended data square with a width of
// edsWidth. All shares are initialized to nil so that the returned extended
// data square can be populated via subsequent SetCell invocations.
func NewExtendedDataSquare(codec Codec, treeCreatorFn TreeConstructorFn, edsWidth uint, chunkSize uint) (*ExtendedDataSquare, error) {
func NewExtendedDataSquare(codec Codec, treeCreatorFn TreeConstructorFn, edsWidth uint, shareSize uint) (*ExtendedDataSquare, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's part of the public API, right? If so this is a breaking change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye. NewExtendedDataSquare is part of the public API so a param name change is technically breaking. I can undo this name change.

In practice, I think this wouldn't actually break any consumers because this function is called with positional arguments so callers will still behave as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, maybe it's not API breaking:

Screenshot 2024-03-11 at 12 06 38 PM

@rootulp
Copy link
Collaborator Author

rootulp commented Mar 11, 2024

Going to merge as is. If we need to be really careful and not introduce any technically API breaking changes, I can revert the few name changes that manifest in the public API docs via a separate follow-up PR.

@rootulp rootulp merged commit d3b3a54 into celestiaorg:main Mar 11, 2024
3 checks passed
@rootulp rootulp deleted the rp/share branch March 11, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent naming: shard, chunk, buffer, share
4 participants