-
Notifications
You must be signed in to change notification settings - Fork 32
Histogram error handling #25
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
Histogram error handling #25
Conversation
src/histogram/strategies.rs
Outdated
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to return Result - let me know what you think @munckymagik @jturner314
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.
@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 Result<Option<_>, _> in some places for consistency with methods that return None for empty input arrays. After seeing how that approach affected this PR, though, I think we should use Result<_, _> instead after all. Doing so simplifies the implementation and public API. There really isn't much disadvantage to returning Result even for things like .mean() (which now returns Result<A, EmptyInput> instead of Option<A>). There should be no performance cost, and it's easy to convert Result<T, E> to Option<T> if needed with the .ok() method on Result. I think I've also addressed all of @munckymagik's comments.
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 Result everywhere I do agree it looks much cleaner (and less nested thanks to ? and error conversions)!
@jturner314 @LukeMathWalker looking really good.
There was just one other comment I made about | vs ||.
Otherwise 🚢 it.
...istogram-error-handling # Conflicts: # src/histogram/strategies.rs
Taken care of that one @munckymagik 👍
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.
Looks ready to merge!
Following on #16, it handles all current panic scenarios for our bin strategies, returning an
Optionwhen a suitable bin collection cannot be generated.