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

implement fmt::Display for Partition #934

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arembridge
Copy link

@arembridge arembridge commented May 6, 2024

Implement the Display functionality for Partition (similar to topic.rs )

PR raised as an initial response to: #481

Please let me know if you are happy with these changes. If so, I can also implement Segment and Stream!

@hubcio
Copy link
Collaborator

hubcio commented May 6, 2024

Looks good, but please also use it whenever possible :)

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 8971001847

Details

  • 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 82.533%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server/src/streaming/partitions/partition.rs 0 8 0.0%
Files with Coverage Reduction New Missed Lines %
sdk/src/binary/system.rs 1 96.97%
sdk/src/cli/client/get_client.rs 1 64.29%
Totals Coverage Status
Change from base Build 8934395468: -0.03%
Covered Lines: 22520
Relevant Lines: 27286

💛 - Coveralls

@@ -249,6 +261,7 @@ mod tests {
Arc::new(AtomicU64::new(0)),
Arc::new(AtomicU32::new(0)),
);
println!("{}", partition);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this, don't introduce new prints, instead try to find the places where we print data about partition and use your implementation

Copy link
Author

Choose a reason for hiding this comment

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

i'm tempted to write assert_eq!("...", format!("{}", partition); at the end of one of the existing tests but feel this isn't what you are referring to? i'm new to rust and haven't had much luck searching the tests for println!(...) or format!(...) across the test suite. i also tried just throwing a panic in the fmt function to see in which tests this would surface, but didn't have much luck...

thanks for your patience helping me with this! i appreciate you could probably knock out this PR in seconds.

Copy link
Author

Choose a reason for hiding this comment

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

so i think i now have my code covered with tests, but i'm not sure it's how you would like it implemented @hubcio ! improvements/recommendations welcome!

@arembridge arembridge force-pushed the impl-display branch 4 times, most recently from 18fa07a to 353d9fa Compare May 8, 2024 09:12
@arembridge arembridge marked this pull request as ready for review May 8, 2024 13:33
@hubcio
Copy link
Collaborator

hubcio commented May 8, 2024

@alexander-rembridge have you already joined our discord? what's ur username?

@arembridge
Copy link
Author

@alexander-rembridge have you already joined our discord? what's ur username?

@rembridge

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.

3 participants