Unrecord #134
Open
Unrecord #134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
requested changes
Jul 26, 2025
| @@ -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.
Suggested change
| /// 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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.