-
-
Notifications
You must be signed in to change notification settings - Fork 456
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
Add WeightedError #547
Add WeightedError #547
Conversation
Okay, looks good to me. Yes, there is logic in returning different types of errors. Oh, |
Didn't realize that |
Pushed an updated PR that should work on 1.22. |
56a339d
to
bc655b5
Compare
Err... I have no idea what's going on. Current builds are failing locally for me due to being unable to find However fixing that in this PR causes test failures with |
Seems to be working now. |
NoItem, | ||
NegativeWeight, | ||
AllWeightsZero, | ||
} |
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.
Some (minimal) docs would be nice.
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'm surprised there wasn't an error about this actually; we use deny(missing_docs)
. But it doesn't look too bad:
https://rust-lang-nursery.github.io/rand/rand/distributions/enum.WeightedError.html
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.
Yes, this is weird. In any case, I opened #550 to fix this.
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.
The reason it wasn't an error is because we have #[doc(hidden)]
for a bunch of stuff in distributions/mod.rs
. They are removed in #548 though.
This fixes #535. I didn't add a
impl From<WeightedError> for Error { .. }
since I'm not sure how that's used, and so didn't know how to write/test an implementation. Happy to add one if anyone has pointers.It's a little irky to have
choose
andchoose_weighted
indicate errors in different ways (one throughOption
and one throughResult
). However negative weights are substantially different than anything thatchoose
has to deal with, so it does make sense.