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
jonhoo
left a comment
jonhoo
left a comment
There was a problem hiding this comment.
Thank you! My guess is that CI will also complain about a change to MSRV, but a slight bump is probably warranted anyway. Main point is that I'd like to split out the lint fixes from the unrecord feature such that they're in separate PRs please.
| // 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?
| /// 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?
| 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)| //! | ||
| //! - 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? |
3 participants
