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

docs: clarify usage of len(merkleRowRoots) #2431

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

rootulp
Copy link
Contributor

@rootulp rootulp commented Jul 5, 2023

Closes #2392

Note: this doesn't introduce a new variable name (the proposed solution in the issue). Happy to do so if reviewers request.

@github-actions github-actions bot added the external Issues created by non node team members label Jul 5, 2023
@rootulp rootulp self-assigned this Jul 5, 2023
@rootulp rootulp added the kind:docs For solely documentation PRs label Jul 5, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2431 (1d77fe7) into main (beaf6db) will decrease coverage by 0.27%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2431      +/-   ##
==========================================
- Coverage   53.28%   53.01%   -0.27%     
==========================================
  Files         156      156              
  Lines        9915     9919       +4     
==========================================
- Hits         5283     5259      -24     
- Misses       4179     4205      +26     
- Partials      453      455       +2     
Impacted Files Coverage Δ
share/eds/byzantine/bad_encoding.go 32.74% <0.00%> (-3.96%) ⬇️

... and 5 files with indirect coverage changes

@rootulp
Copy link
Contributor Author

rootulp commented Jul 5, 2023

hmm is the Required Labels check failing here because the label is kind:doc and it wants kind:docs 🤔

@Wondertan
Copy link
Member

Wondertan commented Jul 6, 2023

This might be already covered by two overs PRs targeting BEFP

cc @vgonkivs

@vgonkivs
Copy link
Member

vgonkivs commented Jul 6, 2023

Hey @rootulp, thanks for the PR. I've already changed the order in this PR, but the comment is really helpful.

@rootulp
Copy link
Contributor Author

rootulp commented Jul 6, 2023

@vgonkivs should I keep this PR open or close it in favor of #2408?

@vgonkivs
Copy link
Member

vgonkivs commented Jul 6, 2023

You can revert the condition but keep your comment. wdyt?

@rootulp
Copy link
Contributor Author

rootulp commented Jul 6, 2023

You can revert the condition but keep your comment. wdyt?

Done in 36e7bb7

@rootulp rootulp added kind:docs For solely documentation PRs and removed kind:docs For solely documentation PRs labels Jul 6, 2023
@rootulp
Copy link
Contributor Author

rootulp commented Jul 7, 2023

@vgonkivs @Wondertan this PR is ready for review + merge.

Wondertan
Wondertan previously approved these changes Jul 8, 2023
@rootulp rootulp enabled auto-merge (squash) July 13, 2023 14:02
@rootulp rootulp merged commit eea0668 into celestiaorg:main Jul 13, 2023
@rootulp rootulp deleted the rp/docs-merkle-roots branch July 13, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fraud external Issues created by non node team members kind:docs For solely documentation PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a new variable for clarity
6 participants