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

chore: improve request prepare proposal comments #1393

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jun 13, 2024

Closes #1390

@rootulp rootulp self-assigned this Jun 13, 2024
@rootulp rootulp requested a review from a team as a code owner June 13, 2024 19:24
@rootulp rootulp requested review from cmwaters and staheri14 and removed request for a team June 13, 2024 19:24
@rootulp rootulp marked this pull request as draft June 13, 2024 19:51
@rootulp rootulp marked this pull request as ready for review June 13, 2024 19:54
@rootulp rootulp requested review from evan-forbes and removed request for staheri14 June 13, 2024 19:54
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.

LGTM, just some nits about the field names in the descriptions.

// block_data is an array of transactions that will be included in a block,
// sent to the app for possible modifications.
// applications can not exceed the size of the data passed to it.
// BlockData is a slice of candidate transactions that may be included in a
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
// BlockData is a slice of candidate transactions that may be included in a
// block_data is a slice of candidate transactions that may be included in a

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would rather use BlockData because IMO more people are likely to read the generated Go comments instead of the raw protobuf comments.

Screenshot 2024-06-17 at 10 09 07 AM

tendermint.types.Data block_data = 1;
// If an application decides to populate block_data with extra information, they can not exceed this value.
// BlockDataSize is the maximum size (in bytes) that BlockData should be.
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
// BlockDataSize is the maximum size (in bytes) that BlockData should be.
// block_data_size is the maximum size (in bytes) that BlockData should be.

@rootulp rootulp enabled auto-merge (squash) June 17, 2024 14:09
@rootulp rootulp merged commit 056df6c into celestiaorg:v0.34.x-celestia Jun 18, 2024
19 checks passed
@rootulp rootulp deleted the rp/improve-comments-request-prepare-proposal branch June 18, 2024 18:46
@rootulp rootulp added the S:backport-to-main Mergify should auto backport PRs with this label to target main. label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:backport-to-main Mergify should auto backport PRs with this label to target main.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants