Unrecord #134
Unrecord #134
Conversation
bogzbonny
commented
May 15, 2025
bogzbonny
commented
- added in basic unrecord functionality as per discussions in Feature request: an unrecord() function #114
- cargo update, and clippy fixes
- probably should write some tests
| @@ -228,7 +228,7 @@ fn quantiles<R: BufRead, W: Write>( | |||
| // Normally I frown on excessive use of From as it's too "magic", but in the limited confines of | |||
| // subcommands, the convenience seems worth it. | |||
| #[derive(Debug)] | |||
| enum CliError { | |||
| pub enum CliError { | |||
There was a problem hiding this comment.
I'm not sure I follow why this was made pub?
| @@ -7,7 +7,7 @@ use std::fmt; | |||
| pub enum CreationError { | |||
| /// Lowest discernible value must be >= 1. | |||
| LowIsZero, | |||
| /// Lowest discernible value must be <= `u64::max_value() / 2` because the highest value is | |||
| /// Lowest discernible value must be <= \x60u64::MAX / 2\x60 because the highest value is | |||
There was a problem hiding this comment.
| /// Lowest discernible value must be <= \x60u64::MAX / 2\x60 because the highest value is | |
| /// Lowest discernible value must be <= `u64::MAX / 2` because the highest value is |
There was a problem hiding this comment.
You've used this \x60 version in a few places now — could you change all of them back to backtick please?
| @@ -850,6 +850,14 @@ impl<T: Counter> Histogram<T> { | |||
| self.record_n(value, T::one()) | |||
| } | |||
|
|
|||
| /// Unrecord `value` in the histogram, removing from the value's current count. | |||
There was a problem hiding this comment.
I'd prefer to do this change in a PR that's separate from all the clippy/lint fixes. Could you split them up please?
| fn record_n_inner(&mut self, mut value: u64, count: T, clamp: bool) -> Result<(), RecordError> { | ||
| fn record_n_inner( | ||
| &mut self, | ||
| sub: bool, |
There was a problem hiding this comment.
I think I'd prefer that we add
enum Op {
Add,
Sub,
}as it will make the callsites a lot easier to vet. For example, it'll read:
self.record_n_inner(Op::Add, value, count, true)instead of
self.record_n_inner(false, value, count, true)| @@ -91,20 +91,20 @@ | |||
| //! ends up setting its internal version of StartTime. | |||
| //! | |||
| //! - Neither StartTime nor BaseTime are present: interval timestamps are interpreted as seconds | |||
| //! since the epoch. The first interval's timestamp is stored to the StartTime field. | |||
| //! since the epoch. The first interval's timestamp is stored to the StartTime field. | |||
There was a problem hiding this comment.
Why did the indentation change for all of these lines?
There was a problem hiding this comment.
pretty sure this is just rustfmt
|
Guys, is there a way to move this forward? |
