-
Notifications
You must be signed in to change notification settings - Fork 33
fix(tool): update rand crate to the latest version #216
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
Conversation
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 think you added one file by accident.
crates/binjs_generic/src/pick.rs
Outdated
json::from(string) | ||
} | ||
TypeSpec::Number => { | ||
json::from(rng.next_f64()) | ||
json::from(rng.next_u64()) |
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.
Hey, I want f64
, not u64
:)
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.
sadly, that was removed
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.
This seems to suggest that there's still a way, no?
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.
Okayes, let me do it differently.
rng.gen::<f64>()
would do
yarn.lock
Outdated
@@ -0,0 +1,362 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. |
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.
Wait, why is this updating yarn.lock
?
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.
my yarn version is different, this should not be here I will remove it
.clone() | ||
// rng.choose(&[Identity, Gzip, /* Deflate is apprently broken https://github.com/alexcrichton/flate2-rs/issues/151 , */ Brotli /*, Lzw doesn't work yet */]) | ||
// .unwrap() |
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.
Could you remove that code instead of commenting it out?
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.
My bad 👍
tests/test_roundtrip.rs
Outdated
let float = rng.next_f64(); | ||
float < CHANCES_TO_SKIP | ||
fn should_skip(rng: &mut RngCore) -> bool { | ||
let float = rng.next_u64(); |
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.
Same problem of f64
:)
Since CHANCES_TO_SKIP <= 1
, by using next_u64
, you're effectively skipping only if your result is 0
, which happens roughly 1/2^64 times, i.e. never :)
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.
Done
Could you:
|
177d90d
to
16f34d3
Compare
remove yarn lock file use sliceRandom instead of deprecated choose method remove yarn lock clean and move u64 to f64 rng fix test roundtrip
16f34d3
to
b391855
Compare
@Yoric Done 👍 There are a lot of changes in the rand crate, took a little time to get it. |
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 good to me!
Feel free to merge when you're ready! |
I don't have write access 😢 |
Fixes a part of #165