Skip to content
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

Use RFC 1946 for doc links #691

Merged
merged 3 commits into from
Jan 14, 2019
Merged

Use RFC 1946 for doc links #691

merged 3 commits into from
Jan 14, 2019

Conversation

newpavlov
Copy link
Member

#690

Links should be checked in the generated documentation, as some errors have may slipped through. For non-dependency links I've used links to crates (grep docs.rs links) instead of links directly to items.

Also there are weird warnings for rand_core, if documentation is generated in its folder there is no errors, but if we run cargo doc in the root folder it spits several "[..] cannot be resolved, ignoring it..." errors, but those links are generated correctly...

@dhardy
Copy link
Member

dhardy commented Jan 11, 2019

rust-lang/rust#57488 ... it's the 11th now so safe to try again?

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

There are a lot of intra_doc_link_resolution_failure warnings when building the doc for this — and yet the links do work. Should we turn these warnings off? Is it a rustdoc bug? I'd prefer not to suppress real warnings but really don't want a bunch of spurious ones polluting the output.

I didn't review everything yet — lets discuss these points first.

@@ -16,7 +16,8 @@
//! implementations only need to concern themselves with generation of the
//! block, not the various [`RngCore`] methods (especially [`fill_bytes`], where
//! the optimal implementations are not trivial), and this allows
//! [`ReseedingRng`] perform periodic reseeding with very low overhead.
//! `ReseedingRng` (see [`rand`](https://docs.rs/rand) crate) perform periodic
//! reseeding with very low overhead.
Copy link
Member

Choose a reason for hiding this comment

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

This is my second point in #690. I don't much like it — it depends on docs.rs and even so isn't a direct link — but I guess it's one option.

I would prefer such a link point to the crates.io page, possibly with a second link pointing to the API docs, or as discussed in rust-lang/docs.rs#204, be a direct link like [rand::rngs::adapter::ReseedingRng] (unfortunately I cannot get this to work).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally I would like to [rand::rngs::adapter::ReseedingRng] (or something similar) to work , but AFAIK there is currently no good way to do it except absolute links. So I think it's alright to use docs.rs links as a temporary solution, which will be easy to replace in future.

//! [`RngCore`]: ../trait.RngCore.html
//! [`fill_bytes`]: ../trait.RngCore.html#tymethod.fill_bytes
//! [`ReseedingRng`]: ../../rand/rngs/adapter/struct.ReseedingRng.html
//! [`BlockRngCore`]: crate::block::BlockRngCore
Copy link
Member

Choose a reason for hiding this comment

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

So items from the crate root (RngCore) get auto-linked but not those from the same module (BlockRngCore)?

Copy link
Member Author

@newpavlov newpavlov Jan 13, 2019

Choose a reason for hiding this comment

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

Yes, it's a weird restriction, you can reference imported items, but not defined in the current module. It probably deserves a rustdoc issue.

//! [`Error`]: struct.Error.html
//! [`impls`]: impls/index.html
//! [`le`]: le/index.html
//! [`rand`]: 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.

ditto — this was previously linked to the crate page

Copy link
Member Author

@newpavlov newpavlov Jan 13, 2019

Choose a reason for hiding this comment

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

I think linking docs.rs is a better choice here, because most likely while browsing documentation users will want to read rand docs, and not see the crates.io page, so we will save one click for them.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Okay, looks good (bar a few minor language quibbles)!

rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_isaac/src/isaac.rs Outdated Show resolved Hide resolved
rand_isaac/src/isaac64.rs Outdated Show resolved Hide resolved
src/distributions/weighted.rs Outdated Show resolved Hide resolved
src/rngs/jitter.rs Outdated Show resolved Hide resolved
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.

2 participants