Skip to content

refactor: GeyserPluginManager doesn't need to implement an empty new() when it derives Default#7750

Merged
akhi3030 merged 5 commits intoanza-xyz:masterfrom
akhi3030:default-cleanups
Aug 30, 2025
Merged

refactor: GeyserPluginManager doesn't need to implement an empty new() when it derives Default#7750
akhi3030 merged 5 commits intoanza-xyz:masterfrom
akhi3030:default-cleanups

Conversation

@akhi3030
Copy link
Copy Markdown

@akhi3030 akhi3030 commented Aug 27, 2025

Problem

  • GeyserPluginManager derives default and implements new() with takes no arguments and effectively calls default().

Summary of Changes

  • Removes GeyserPluginManager::new()

@akhi3030 akhi3030 requested a review from bw-solana August 27, 2025 18:08
@akhi3030
Copy link
Copy Markdown
Author

@bw-solana: github didn't recommend any reviewers. Could you maybe recommend someone or review please?

@bw-solana bw-solana requested a review from lijunwangs August 27, 2025 19:33
@bw-solana
Copy link
Copy Markdown

@bw-solana: github didn't recommend any reviewers. Could you maybe recommend someone or review please?

Added @lijunwangs

Comment thread test-validator/src/lib.rs Outdated
warp_slot: Default::default(),
accounts: Default::default(),
upgradeable_programs: Default::default(),
ticks_per_slot: Default::default(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clippy is not happy with this change. To me, I am not sure I like all Default::default either, It is harder to read without actually looking up the data type.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@lijunwangs: thanks for the review. I didn't realise we had such lints enabled.

I can fix it up to what clippy suggests. Would that be acceptable or do you prefer the code as is?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can just make the geyser trivial new --> default change and leaving the rest alone. Code clarity is more important.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, I reverted these changes. PTAL.

@akhi3030 akhi3030 changed the title refactor: various cleanups around how Default::default() are used refactor: GeyserPluginManager doesn't need to implement an empty new() when it derives Default Aug 28, 2025
@akhi3030 akhi3030 requested a review from lijunwangs August 28, 2025 19:01
Copy link
Copy Markdown

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

CI is failing,

Copy link
Copy Markdown

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

175 | geyser_plugin_manager: Arc::new(RwLock::new(GeyserPluginManager::new())),
still some test code using the removed ::new

@akhi3030
Copy link
Copy Markdown
Author

175 | geyser_plugin_manager: Arc::new(RwLock::new(GeyserPluginManager::new())), still some test code using the removed ::new

ah, when I undid the default changes, this crept back in. Should be fixed now.

@akhi3030 akhi3030 requested a review from lijunwangs August 29, 2025 19:10
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.0%. Comparing base (77f600b) to head (f005729).

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7750     +/-   ##
=========================================
- Coverage    83.0%    83.0%   -0.1%     
=========================================
  Files         812      812             
  Lines      357199   357194      -5     
=========================================
- Hits       296801   296732     -69     
- Misses      60398    60462     +64     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@akhi3030 akhi3030 merged commit 5d692bd into anza-xyz:master Aug 30, 2025
43 checks passed
@akhi3030 akhi3030 deleted the default-cleanups branch October 29, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants