Skip to content
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

Merged
merged 1 commit into from Jul 12, 2018
Merged

Add WeightedError #547

merged 1 commit into from Jul 12, 2018

Conversation

sicking
Copy link
Contributor

@sicking sicking commented Jul 12, 2018

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 and choose_weighted indicate errors in different ways (one through Option and one through Result). However negative weights are substantially different than anything that choose has to deal with, so it does make sense.

@dhardy
Copy link
Member

dhardy commented Jul 12, 2018

Okay, looks good to me. Yes, there is logic in returning different types of errors.

Oh, Error::description is "soft deprecated"? Strange. I'm not too against updating the min Rustc version but it should be easy enough to support 1.22 for now.

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Didn't realize that Error::description deprecation had happened since 1.22. Definitely not a reason to update rustc for that.

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Pushed an updated PR that should work on 1.22.

@sicking sicking force-pushed the weighterr branch 4 times, most recently from 56a339d to bc655b5 Compare July 12, 2018 12:15
@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Err... I have no idea what's going on. Current builds are failing locally for me due to being unable to find alloc::collections::BTreeMap. Which makes sense since per https://doc.rust-lang.org/alloc/btree_map/struct.BTreeMap.html BTreeMap lives in alloc::btree_map::BTreeMap.

However fixing that in this PR causes test failures with Could not find "btree_map" in "alloc".

@sicking
Copy link
Contributor Author

sicking commented Jul 12, 2018

Seems to be working now.

@dhardy dhardy merged commit c032204 into rust-random:master Jul 12, 2018
NoItem,
NegativeWeight,
AllWeightsZero,
}
Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ErrorKind::Input
3 participants