Skip to content

A bit more work on the readme #437

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

Merged
merged 2 commits into from
May 15, 2018
Merged

Conversation

pitdicker
Copy link
Contributor

@pitdicker pitdicker commented May 11, 2018

Changes:

  • A little more of a description at the start of the readme.
  • Link to rand_core less prominently.
  • Show the prelude and random();
  • Mention 'Functionality' earlier, order the functionality by importance (for users), and mention rand_core less here.
  • Remove the section about 'Compatibility shims'. I think it is better to open an issue for that, than to have it in the readme.
  • Some rewording around crate features.
  • I don't see much value in the 'Testing' section.

Rendered.

README.md Outdated
* [rand_core](https://crates.io/crates/rand_core)
Part of the functionality of Rand lives in the [rand_core](
https://crates.io/crates/rand_core) crate, which is not intended for users but
primarily for crates implementing RNGs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this style much. We will probably have more sub-crates shortly so a list is a good presentation, and then a description something like:

core generation traits; this crate is most useful when implementing RNGs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the text a bit, but shall we wait switching to a list until it contains more than one item?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like. But you're the one switching here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never really understood the bullet point, it seemed to be for some kind of stylistic emphasis. Only now I get it was meant to be the first item of what should become a list.

README.md Outdated
[master branch](https://rust-lang-nursery.github.io/rand/rand/index.html),
[by release](https://docs.rs/rand)
[by release](https://docs.rs/rand).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering what docs.rs is doing at the moment, it may be best to link directly to the current minor version; i.e. docs.rs/rand/0.5 for the next release (doesn't work at the moment, but 0.4 does so should be fine for the release).

README.md Outdated
use rand::prelude::*;

// basic usage with random():
let x: u8 = rand::random();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need rand:: prefix with the prelude

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we advertise random so prominently, shouldn't we also mention the caveats?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which do you have in mind to mention?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That calling random often is not recommended, because it is more efficient to reuse the RNG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not here, no: (1) premature optimisation (actually there's not a big overhead anyway); (2) it's in the random doc and keeps this simple.

README.md Outdated
- Functionality for seeding PRNGs: the `FromEntropy` trait, and as sources of
external randomness `EntropyRng`, `OsRng` and `JitterRng`.
- Most content from `rand_core` (re-exported): base random number generator
traits and error-reporting types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to say this without explaining what rand_core is? Not sure whether there should be a section in this README; otherwise maybe link to https://crates.io/crates/rand_core.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is linked above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

README.md Outdated

There is a compatibility shim from 0.3 to 0.4 forcibly upgrading all Rand 0.3
users; this is largely due to the small differences between the two versions.

### Rust version requirements

The 0.5 release of Rand will require **Rustc version 1.22 or greater**.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well get this right for the actual release now? will requirerequires; also maybe remove the below?

README.md Outdated
functionality depending on `std`:

- `thread_rng()`, and `random()` are not available, as they require thread-local
storage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and an entropy source

README.md Outdated

- `thread_rng()`, and `random()` are not available, as they require thread-local
storage.
- `OsRng` is unavailable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and EntropyRng

Is it worth linking to #313 somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. Is it important enough for the readme?

@dhardy
Copy link
Member

dhardy commented May 11, 2018

Actually, better not to merge until we're ready to release 0.5 because until then the doc link is broken.

@dhardy dhardy merged commit a0303ab into rust-random:master May 15, 2018
@pitdicker pitdicker deleted the simplify_readme branch June 8, 2018 18:27
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.

3 participants