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

Do not return unused values from wantlists #792

Merged
merged 4 commits into from
Jan 17, 2025
Merged

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Jan 17, 2025

  • Remove unnecessary setting empty dict to nil
  • Do not return unused value from Remove or from Contains
  • Do not export functions unnecessarily.

Skip changelog because there are not functional changes here.

@gammazero gammazero requested a review from a team as a code owner January 17, 2025 05:08
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.46%. Comparing base (b62b60e) to head (3263926).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...tswap/client/internal/messagequeue/messagequeue.go 93.10% 0 Missing and 2 partials ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
- Coverage   60.53%   60.46%   -0.07%     
==========================================
  Files         245      245              
  Lines       31139    31132       -7     
==========================================
- Hits        18849    18823      -26     
- Misses      10615    10628      +13     
- Partials     1675     1681       +6     
Files with missing lines Coverage Δ
...ernal/blockpresencemanager/blockpresencemanager.go 93.18% <100.00%> (-3.63%) ⬇️
bitswap/client/wantlist/wantlist.go 90.62% <100.00%> (-0.29%) ⬇️
...tswap/client/internal/messagequeue/messagequeue.go 83.39% <93.10%> (-2.05%) ⬇️

... and 8 files with indirect coverage changes

The presence map in blockPresenceManager is never empty during normal use, so do not bother setting the map to nil for CG is its size becomes zero.
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

LGTM

The change is breaking Wantlist compatibility (Remove, Contains), but I guess there aren't many users anyway

@gammazero
Copy link
Contributor Author

@guillaumemichel I think it is safe to assume that no one is using the bitswap/client/wantlist package outside of this code.

@gammazero gammazero merged commit 8ca0ca2 into main Jan 17, 2025
17 checks passed
@gammazero gammazero deleted the wantlist-returns branch January 17, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants