implement fmt::Display for Partition #934
Conversation
|
Looks good, but please also use it whenever possible :) |
Pull Request Test Coverage Report for Build 8971001847Details
💛 - Coveralls |
| @@ -249,6 +261,7 @@ mod tests { | |||
| Arc::new(AtomicU64::new(0)), | |||
| Arc::new(AtomicU32::new(0)), | |||
| ); | |||
| println!("{}", partition); | |||
There was a problem hiding this comment.
remove this, don't introduce new prints, instead try to find the places where we print data about partition and use your implementation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
impl-display
branch
4 times, most recently
from
18fa07a
to
353d9fa
Compare
|
@alexander-rembridge have you already joined our discord? what's ur username? |
|
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
SegmentandStream!