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.

Sign up for GitHub

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

Jump to bottom

refactoring(share/ipld)!: rework getLeavesByNamespace #1870

Merged
merged 10 commits into from
Mar 24, 2023
Merged

refactoring(share/ipld)!: rework getLeavesByNamespace #1870

merged 10 commits into from
Mar 24, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Mar 7, 2023 •
edited
Loading

23d52b30879dbf725cba674ecfe228b2

Overview

This is the first task in the scope of #1835.
Resolves #1836

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added area:shares Shares and samples area:blob kind:refactor Attached to refactoring PRs labels Mar 7, 2023
@vgonkivs vgonkivs self-assigned this Mar 7, 2023
@vgonkivs vgonkivs force-pushed the rework_get_leaves_by_namespace branch 2 times, most recently from 28284dd to 79d00ee Compare March 10, 2023 11:50
@vgonkivs vgonkivs marked this pull request as ready for review March 10, 2023 11:53
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2023 •
edited
Loading

Codecov Report

Merging #1870 (dfbd558) into main (f99fb22) will decrease coverage by 2.79%.
The diff coverage is 80.91%.

@@            Coverage Diff             @@
##             main    #1870      +/-   ##
==========================================
- Coverage   57.31%   54.52%   -2.79%     
==========================================
  Files         242      215      -27     
  Lines       16149    13690    -2459     
==========================================
- Hits         9255     7464    -1791     
+ Misses       5946     5439     -507     
+ Partials      948      787     -161
Impacted Files Coverage Δ
share/ipld/proof_collector.go 100.00% <ø> (ø)
share/get.go 91.42% <75.00%> (+0.80%) ⬆️
share/ipld/namespace_data.go 77.77% <77.77%> (ø)
share/ipld/get.go 93.71% <85.71%> (+1.57%) ⬆️
share/getter.go 78.78% <100.00%> (-5.31%) ⬇️
share/getters/utils.go 75.38% <100.00%> (+3.45%) ⬆️
share/p2p/shrexnd/client.go 56.77% <100.00%> (-0.37%) ⬇️
share/p2p/shrexnd/server.go 60.86% <100.00%> (-2.55%) ⬇️

... and 104 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vgonkivs vgonkivs force-pushed the rework_get_leaves_by_namespace branch from 6f27ae6 to e7d2a60 Compare March 10, 2023 12:10
@vgonkivs vgonkivs mentioned this pull request Mar 10, 2023
Closed
Wondertan
Wondertan reviewed Mar 13, 2023
View reviewed changes
share/ipld/get.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs mentioned this pull request Mar 13, 2023
Closed
5 tasks
@vgonkivs vgonkivs force-pushed the rework_get_leaves_by_namespace branch from a19d709 to 50cd8e7 Compare March 13, 2023 15:17
@vgonkivs vgonkivs force-pushed the rework_get_leaves_by_namespace branch from 50cd8e7 to d754afb Compare March 13, 2023 15:37
@vgonkivs vgonkivs requested a review from Wondertan March 13, 2023 15:39
renaynay
renaynay reviewed Mar 16, 2023
View reviewed changes
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/get.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd suggested changes Mar 17, 2023
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

Really love the functional option approach

share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Mar 20, 2023
View reviewed changes
renaynay
renaynay previously approved these changes Mar 20, 2023
View reviewed changes
@renaynay renaynay changed the title refactoring(share/ipld): rework getLeavesByNamespace refactoring(share/ipld)!: rework getLeavesByNamespace Mar 20, 2023
@renaynay
Copy link
Member

This is still technically breaking bc you're changing the API at the share pkg level -- can you mention that in description and also in title?

@vgonkivs vgonkivs added the kind:break! Attached to breaking PRs label Mar 20, 2023
@vgonkivs
Copy link
Member Author

This is still technically breaking bc you're changing the API at the share pkg level -- can you mention that in description and also in title?

Added the breaking label.

@Wondertan
Copy link
Member

Very nice review @renaynay and good suggestions @distractedm1nd

@vgonkivs vgonkivs dismissed stale reviews from renaynay and distractedm1nd via 2e962fd March 22, 2023 16:52
Wondertan
Wondertan previously approved these changes Mar 23, 2023
View reviewed changes
Wondertan
Wondertan previously approved these changes Mar 23, 2023
View reviewed changes
renaynay
renaynay previously approved these changes Mar 23, 2023
View reviewed changes
walldiss
walldiss reviewed Mar 23, 2023
View reviewed changes
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Great idea to make NamespaceData container! 👍

share/ipld/get.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/ipld/namespace_data.go Outdated Show resolved Hide resolved
share/p2p/shrexnd/client.go Show resolved Hide resolved
share/ipld/namespace_data.go Show resolved Hide resolved
share/get_test.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs merged commit ba75307 into celestiaorg:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walldiss walldiss walldiss approved these changes

@distractedm1nd distractedm1nd distractedm1nd approved these changes

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

Assignees

@vgonkivs vgonkivs

Labels
area:blob area:shares Shares and samples kind:break! Attached to breaking PRs kind:refactor Attached to refactoring PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

share: GetLeavesByNamespace should allow returning only proofs without data
6 participants
@vgonkivs @codecov-commenter @renaynay @Wondertan @walldiss @distractedm1nd