-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
generate_range panics if the upper bound is 0 #24
Comments
I mean, there's an assert anyways: https://github.com/aspenluxxxy/nanorand-rs/blob/7e1704e6a80697b7b4bda4d93a8a476748379463/nanorand/src/gen.rs#L49-L51 and there's currently no support for generating signed integers anyways, so this is expected behavior? where are your running into this? |
I ran into it on accident because I was trying to pick a random index of a |
nanorand has some bugs and weirdness that make me not fully trust it Absolucy/nanorand-rs#24 Absolucy/nanorand-rs#21 for a fundamental thing like randomness i'd rather use a battle-tested lib
it should no longer panic. |
If I am reading the code correctly, it’ll now return the lower bound for an empty range. I wonder if that’s the desired behavior? Looking at the prior art, rand, fastrand and Python’s random all panic on empty ranges. At a first glance, panicking does make sense, and violating Perhaps just the panic error message needs to be adjusted? It seems that (guessing here, the report isn’t supper clear) this is what tesselode is asking for, it’s not “make it non panic by returning something arbitrary”. |
Yeah, looking back, this was not a good issue report. An invalid range should cause a panic, it should just have a proper error message. I think I was confused about whether this should panic or not because the error message looked like it was caused by an internal bug, not user error. |
Example:
Produces this output:
The text was updated successfully, but these errors were encountered: