Histogram error handling #25
Conversation
src/histogram/strategies.rs
Outdated
| /// **Panics** if `bin_width<=0`. | ||
| fn new(bin_width: T, min: T, max: T) -> Self | ||
| /// Returns `None` if `bin_width<=0` or `min` >= `max`. | ||
| fn new(bin_width: T, min: T, max: T) -> Option<Self> |
There was a problem hiding this comment.
Question: is returning no object really a valid outcome or should it actually return a Result<Self, WhateverError> to signify a failure?
Hi, curious/lurking reviewer here. Saw the first "adventure" blog post and ended up here. Really great work that you people are doing and an inspiring article/tutorial.
If returning None is genuinely a valid, non-exceptional outcome, given certain args, then maybe a function name other than new would help clarify it's behavior? Just a suggestion.
There was a problem hiding this comment.
A Result is indeed a better return type there - I've been lazy because EquiSpaced is not part of the public API, but that's not a reason to be sloppy.
The creation method for the public strategies is called from_array: do you think an Option is a good output type or would rather see a Result as a user?
There was a problem hiding this comment.
I totally understand 👍
As for the public from_array I'm hesitant to give a strong opinion yet, because I've only just started looking through the code. But the general rule of thumb I would apply is this:
- Use an
Optionif returningNonemeans it's ok to continue - If it's not ok to continue, the user may only see a problem later down the line, when a "symptomatic" error occurs rather than the actual cause.
- With
Nonethat symptom may just be no-output.
Is that any help?
There was a problem hiding this comment.
It totally is - going again through the code I think it makes sense to convert the output type to Result. I'll work on it 👍
Thanks for your input @munckymagik!
There was a problem hiding this comment.
Refactored to return Result - let me know what you think @munckymagik @jturner314
There was a problem hiding this comment.
@LukeMathWalker it's looking good 👍 . Sorry for the delay in reviewing. Have left a few comments for you to consider.
My overall gut feeling is we should turn StrategyError into something like ShapeError in ndarray. We could use our own ErrorKind enum to represent cases like constant or empty array errors.
It would provide explicit feedback to users should they want it, and give future committers an easy path to fall-into to add new errors without breaking changes.
|
I've proposed some changes in LukeMathWalker#4. In #24, we decided to use |
This is nice because it doesn't lose information. (Returning an `Option` combines the two error variants into a single case.)
Once the `#[non_exhaustive]` attribute is stable, we should replace the hidden enum variant with that attribute on the enum.
Improve error handling
|
Looking at the code in LukeMathWalker#4 after the shift to |
|
@jturner314 @LukeMathWalker looking really good. There was just one other comment I made about Otherwise 🚢 it. |
…istogram-error-handling # Conflicts: # src/histogram/strategies.rs
|
Taken care of that one @munckymagik 👍 |
Following on #16, it handles all current panic scenarios for our bin strategies, returning an
Optionwhen a suitable bin collection cannot be generated.