-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
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.
My suggestions are optional, the PR as it is LGTM!
gah tests are failing because https://github.com/celestiaorg/rsmt2d/actions/runs/8207511252/job/22448943530?pr=306#step:6:17 |
@@ -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) { |
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.
This looks like it's part of the public API, right? If so this is a breaking change.
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.
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.
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.
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. |
Closes #215