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

remove pub from to_packet_batches_for_tests #31056

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Apr 4, 2023

Problem

This shouldn't be pub because it is marked as cfg(test) and is generic.
If you try to use this outside of the crate, it leads to confusing messaging about the fn not existing.

Summary of Changes

Remove pub

Fixes #

@apfitzge
Copy link
Contributor Author

apfitzge commented Apr 4, 2023

A relevant rust-lang feature request PR: rust-lang/cargo#8379

@apfitzge apfitzge requested a review from steviez April 4, 2023 23:22
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #31056 (8f48bcc) into master (399b7ba) will increase coverage by 0.0%.
The diff coverage is 96.5%.

@@           Coverage Diff            @@
##           master   #31056    +/-   ##
========================================
  Coverage    81.5%    81.5%            
========================================
  Files         728      728            
  Lines      205473   205547    +74     
========================================
+ Hits       167525   167630   +105     
+ Misses      37948    37917    -31     

Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM - can always re-promote to pub(crate) later if we end up wanting it somewhere else in this crate

@apfitzge apfitzge merged commit 65d9238 into solana-labs:master Apr 5, 2023
@apfitzge apfitzge deleted the clean_up/remove_pub branch April 5, 2023 15:57
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.

2 participants