-
Notifications
You must be signed in to change notification settings - Fork 41
Error enums #22
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
Error enums #22
Conversation
This is on top of #16. |
src/lib.rs
Outdated
/// Number of significant digits must be between 0 and 5 | ||
SigFigTooBig, | ||
/// Cannot represent sigfig worth of values beyond low | ||
CannotRepresentSigFigBeyondLow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect most people will probably not match each of these variants but at least it will be nicely specific if their usage panics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. I think we might be able to bikeshed some better names for the variants, but it's probably not worth the time.
I do think we should give some better documentation for the variants though. This will be one of the main places where people will look for an explanation of why something didn't work, so we should make sure things are as clear as possible. For example, why can't SigFig be bigger than 5, why must high be at least twice of low, etc. For some of these we have explanations elsewhere in the code, and I think those can be moved here.
Also, a minor point, but you should enclose code things (like u64::max_value() / 2
) in backticks.
pub enum AdditionError { | ||
/// The other histogram includes values that do not fit in this histogram's range. | ||
/// Only possible when auto resize is disabled. | ||
OtherAddendValuesExceedRange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, "addend" refers to both terms in an addition. The first one is called the "augend" but that's not what we care about. Oddly this value thing means that addition of non-resizing histograms is not commutative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, interesting. I think that's probably fine though. If I remember correctly, this is the reason I implement only AddAssign
, and not Add
.
/// The other histogram includes counts that are higher than the current count for a value, and | ||
/// counts cannot go negative. The subtraction may have been partially applied to some counts as | ||
/// this error is returned when the first impossible subtraction is detected. | ||
SubtrahendCountExceedsMinuendCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For subtraction we can have maximally precise (or pedantic) names, though!
3115a78
to
942f4f4
Compare
This addresses #10. |
Looking good! I'd like some better docs for the |
I took the liberty of self-bikeshedding on enum names, and also centralized the docs for the |
Looks good! |
No description provided.