Some more documentation work #366

pitdicker · 2018-04-01T14:16:18Z

With #![doc(test(attr(allow(unused_variables), deny(warnings))))] we now make sure all doctest warnings do not get ignored, see Use imported thread_rng() in main doc example. #363.
I made small tweaks to the documentation: linkified some things, shortened the initial description of some structs, less or more documentation for re-exports.
Better documentation and examples for JitterRng.
Reordered the methods of Rng, so they show up in a somewhat more logical order in the documentation.
Removed hard-coded links to rand_core on docs.rs. Because Rand re-exports the things that are linked to, we can just link to the documentation of the re-exports.

I think this is best to review per commit.

dhardy · 2018-04-01T16:35:40Z

src/distributions/range.rs

@@ -88,6 +88,7 @@ impl<X: SampleRange> Distribution<X> for Range<X> {
 /// Helper trait for creating objects using the correct implementation of
 /// `RangeImpl` for the sampling type; this enables `Range::new(a, b)` to work.
 pub trait SampleRange: PartialOrd+Sized {
+    /// Actual `RangeImpl` implementation for `X`.


I'd prefer you didn't use 'actual'; try "The RangeImpl implementation supporting type X."

Also, it would make sense now to rename X → T and T to something else (perhaps Impl; it's not something users will see a lot). This is what Huon suggested I think.

That sounds better, as always.

dhardy · 2018-04-01T16:46:42Z

src/lib.rs

 #![deny(missing_debug_implementations)]
+#![doc(test(attr(allow(unused_variables), deny(warnings))))]


I generally prefer warnings to errors (#![warn(...)]) simply because new compiler versions may add warnings without considering backwards compatibility, thus breaking builds. In the latter case unfortunately we have no choice since warnings from examples are not shown; also "missing docs" is not something the compiler can break so that's fine.

Good point, I'll keep that in mind!

dhardy · 2018-04-01T16:48:12Z

src/lib.rs

-#[cfg(feature="std")] pub use os::OsRng;
+#[doc(inline)] pub use jitter::JitterRng;
+#[cfg(feature="std")]
+#[doc(inline)] pub use os::OsRng;


Keep the OsRng import on a single line please; this causes chaos with diff tools.

dhardy · 2018-04-01T16:54:07Z

src/lib.rs

@@ -489,61 +388,101 @@ pub trait Rng: RngCore {
        Range::sample_single(low, high, self)
    }

-    /// Return a bool with a 1 in n chance of true
+    /// Sample a new value, using the given distribution.


This commit is just reordering, and this is the diff tool getting confused?

Yes, I made sure not to change anything but the order.

dhardy · 2018-04-01T17:29:19Z

Needs rebase

These traits/methods etc. are re-exported by Rand, so we can just link to ours.

pitdicker · 2018-04-01T17:49:45Z

That rebase was harder than I thought 😄. I removed the commit that reordered the Rng methods, and recreated it at the end, to make sure I did not lose changes during the rebase.

pitdicker · 2018-04-02T08:05:49Z

Added three more commits, that may need some explanation.

cargo-deadlinks proved really useful to make sure all documentation links are valid. At least for rand, there are two links in rand_core I don't know what to do with. I have added it to the CI, but don't know if installing cargo-deadlinks will take too much time.

I set the dependency to rand_core to a path in the repro so it uses the fixed links. When rand_core 0.1 is released, everything should work out and rand master can again depend on the crates.io version. But it seems like a good idea to me to depend on a relative path on master sometimes.

The line "[rand_core::impls]: ../rand_core/impls/index.html" has the link go out of the rand_core crate, and then back in. This way it also works for all the re-exported functions in rand. At least locally and on GitHub pages. I added a section to cargo.toml to give hints to docs.rs (documentation), so it hopefully builds rand_core together with the rand_crate.

Finally I tried to clean up the list of exported items in libs.rs. At least in the documentation by hiding things, not by breaking the API. A few more things are now directly available like ReadRng and ReseedingRng, instead of under a module with only one item.

Two exceptions: I broke JitterRng by only exporting the module (couldn't get the links to work from both places). We need to have the module public, because it also contains an error type. And I think not having it at the top level import can even be the right thing... Found one very recent dependency on GitHub that uses it directly.

Also the reseeding module is now not exported any more. We can basically consider it a rewrite, and all existing code using it will have to update. No point in making sure the export does not break 😄.

One oddity: even when we build the documentation with "--all-features" XorShiftRng and IsaacRng don't show they implement Serialize and Deserialize. This is due to rust-lang/rust#36922.

dhardy

I see a couple more significant import changes too:

no longer expose Hc128Rng directly; 👍
make reseeding module private and expose ReseedingRng directly; breaking (except any usage will be broken anyway); not quite sure about this

I'd have preferred it if you'd brought up the breaking changes in a separate issue/PR. The only one I'm really concerned about is the read module.

dhardy · 2018-04-02T08:07:32Z

.travis.yml

@@ -34,6 +34,8 @@ matrix:
        - cargo test --features serde-1,log,nightly
        - cargo test --benches
        - cargo doc --no-deps --all --all-features
+        - cargo --list | egrep "^\s*deadlinks$" -q || cargo install cargo-deadlinks
+        - cargo deadlinks


I've not used it but apparently it only checks local links by default. Is this what we want?

Yes, only the documentation cross-links. But I suppose checking all links can be useful...

dhardy · 2018-04-02T08:11:51Z

src/lib.rs

 pub use rand_core::{RngCore, BlockRngCore, CryptoRng, SeedableRng};
 pub use rand_core::{ErrorKind, Error};

-// external rngs
-#[doc(inline)] pub use jitter::JitterRng;


No longer exporting JitterRng is a breaking change, but I approve.

dhardy · 2018-04-02T08:16:17Z

src/lib.rs

 #[cfg(feature="std")] pub use entropy_rng::EntropyRng;
+#[cfg(feature="std")] pub use os::OsRng;
+#[cfg(feature="std")] pub use read::ReadRng;


This is more convenient but still a significant change. Do we want it? Potentially a Read wrapper (#254) could also live in rand::read.

pitdicker · 2018-04-02T08:25:13Z

I'd have preferred it if you'd brought up the breaking changes in a separate issue/PR.

Yes, that would have been better. It was kind of necessary to make the links checking work out, so it would have been hard to separate.

Mostly just hide, to avoid API breakage.

pitdicker · 2018-04-02T08:35:36Z

Updated the commits.

Some more documentation work

pitdicker force-pushed the deny_doctest_warnings branch from 03312b4 to df0ee65 Compare April 1, 2018 14:48

dhardy approved these changes Apr 1, 2018

View reviewed changes

pitdicker force-pushed the deny_doctest_warnings branch from df0ee65 to 208fee3 Compare April 1, 2018 17:15

pitdicker added 8 commits April 1, 2018 19:42

Deny warnings in doctests

08fd152

Deny missing docs

6d87a6a

Make documentation of re-exports nicer

4c7cdc5

Small doc tweaks

4323e0e

Improve JitterRng documentation

5f47ebf

Remove hardcode docs.rs links to rand_core

5680f6f

These traits/methods etc. are re-exported by Rand, so we can just link to ours.

Rename SampleRange::T to SampleRange:Impl

9dab45b

Reorder Rng methods in order of importance (for documentation)

a70e32e

pitdicker force-pushed the deny_doctest_warnings branch from 208fee3 to a70e32e Compare April 1, 2018 17:47

pitdicker force-pushed the deny_doctest_warnings branch 3 times, most recently from ac4290a to dc381cf Compare April 2, 2018 08:01

dhardy reviewed Apr 2, 2018

View reviewed changes

pitdicker added 3 commits April 2, 2018 10:30

Fix dead links

c008e5f

Clean up public exports in documentation

206583e

Mostly just hide, to avoid API breakage.

Give hints to docs.rs

f953c4b

pitdicker force-pushed the deny_doctest_warnings branch from dc381cf to f953c4b Compare April 2, 2018 08:32

pitdicker mentioned this pull request Apr 2, 2018

Rename uniform #369

Merged

pitdicker merged commit ba88dad into rust-random:master Apr 2, 2018

pitdicker deleted the deny_doctest_warnings branch April 2, 2018 14:00

pitdicker added a commit that referenced this pull request Apr 4, 2018

Merge pull request #366 from pitdicker/deny_doctest_warnings

4e67bd5

Some more documentation work

pitdicker mentioned this pull request May 8, 2018

Implement std::io::Read for RngCore #434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some more documentation work #366

Some more documentation work #366

pitdicker commented Apr 1, 2018

dhardy Apr 1, 2018

pitdicker Apr 1, 2018

dhardy Apr 1, 2018

pitdicker Apr 1, 2018

dhardy Apr 1, 2018

dhardy Apr 1, 2018

pitdicker Apr 1, 2018

dhardy commented Apr 1, 2018

pitdicker commented Apr 1, 2018

pitdicker commented Apr 2, 2018 •

edited

Loading

dhardy left a comment

dhardy Apr 2, 2018

pitdicker Apr 2, 2018

dhardy Apr 2, 2018

dhardy Apr 2, 2018

pitdicker commented Apr 2, 2018

pitdicker commented Apr 2, 2018

		#![deny(missing_debug_implementations)]
		#![doc(test(attr(allow(unused_variables), deny(warnings))))]

Some more documentation work #366

Some more documentation work #366

Conversation

pitdicker commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Apr 1, 2018

pitdicker commented Apr 1, 2018

pitdicker commented Apr 2, 2018 • edited Loading

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Apr 2, 2018

pitdicker commented Apr 2, 2018

pitdicker commented Apr 2, 2018 •

edited

Loading