Remove dependency on system randomness

[reknih]

As discussed in #74, here is a PR that makes system randomness an optional (but enabled by default) feature.

The catch of this PR is that it interacts badly with the test for the example. cargo build --no-default-features works as intended, but cargo test --no-default-features will fail unless the code in the main function of examples/lipsum.rs is removed. Furthermore, I had to adapt one documentation example such that it would work without thread_rng. There are methods to exclude examples from doctesting depending on the features the crate is built with, but they are ugly.

I'd be happy to incorporate any feedback on naming / test architecture. Have a nice day!

[mgeisler]

The catch of this PR is that it interacts badly with the test for the example. cargo build --no-default-features works as intended, but cargo test --no-default-features will fail unless the code in the main function of examples/lipsum.rs is removed.

Right, we should make sure everything builds in all the combinations.

What about an alternative? With the default Cargo features, lipsum will use rand::thread_rng and if you disable the new Cargo feature, lipsum will simply use ChaCha20Rng::seed_from_u64(0) instead (as you already did in the docstring).

Would that not be simpler overall? The API would stay unchanged and the small utility functions like lipsum_title can keep working.

Basically, all you need to do is to create a common function which is used to get the default RNG if rand::thread_rng is not available and then call this function everywhere that rand::thread_rng is called today. We should also update the docstrings to tell the user that the output is random but deterministic if the Cargo feature is disabled.

There are methods to exclude examples from doctesting depending on the features the crate is built with, but they are ugly.

Yeah, I agree, that seems overkill for us.

[mgeisler]

A final thing, please add the magic Fixes #74. line to the commit message so that the issue is closed automatically when the PR is merged.

[mgeisler]

What about an alternative? With the default Cargo features, lipsum will use rand::thread_rng and if you disable the new Cargo feature, lipsum will simply use ChaCha20Rng::seed_from_u64(0) instead (as you already did in the docstring).

I just had an even better idea (perhaps): we could use ChaCha20Rng by default and ask people to call generate_with_rng if they want to use something else. So we could remove any trace of thread_rng from this crate and let people inject it themselves it really needed.

[reknih]

Thank you for your review!
I have removed the direct usage of thread_rng.

I have also retooled the _from_seed methods into _with_rng variants. They now serve a different role: As the crate now is deterministic by default, they allow convenient insertion of a thread_rng, as demonstrated in the documentation. The use case of custom seeds is still served through this function, as the test capitalize_after_punctiation shows.

Those methods take impl Rng as their RNG argument right now. Would you prefer Box<dyn Rng>?

[mgeisler]

Hey Martin, thanks for the update! It looks good, I'll try to get to a proper view in a few days.

[mgeisler]

Those methods take impl Rng as their RNG argument right now. Would you prefer Box<dyn Rng>?

I think impl Rng is fine here. I would expect people to use use this with a single RNG so there should be no real code bloat from the instantiation of the generic type parameter.