-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
More intra-doc links, add explicit exception list to linkchecker #74485
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
This won't actually fix #32553 because it is about the missing |
@ollie27 oh heh I just copied the issue number from the source when I deleted the linkcheck line |
7ee0f2a
to
a968093
Compare
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.
Thanks for working on this!
/// # Examples | ||
/// | ||
/// You can create a `String` from a literal string with [`String::from`]: | ||
/// | ||
/// [`String::from`]: From::from |
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.
Let's link to the relevant impl, not just the trait (this is &'_ str
)
/// [`String::from`]: From::from | |
/// [`String::from`]: #impl-From<%26%27_%20str> |
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.
the exact impl doesn't have special docs and the point is to reduce the direct links :)
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 linking to the type is helpful though. It's not clear that 'literal string' means &'_ str
, especially to beginners.
Also, same comment as below about fragments loading faster than different pages.
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.
We can just fix that with an &str
link :)
/// [`with_capacity_and_hasher`]: #method.with_capacity_and_hasher | ||
/// [`RefCell`]: crate::cell::RefCell | ||
/// [`Cell`]: crate::cell::Cell | ||
/// [`default`]: Default::default |
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 now goes to the trait page, instead of the list of implementations. I know HashMap::default
doesn't currently work, can we keep #method.default
until then? We don't have the ambiguity of String::from because there can only be one Default impl.
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.
IMO the trait page is fine but I'd rather get rid of the #s and have links work over having them link to the trait impl instead of the trait. I don't see a difference in utility there
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.
The difference is that going to the trait takes you to a new page and away from the docs you were originally reading. #method.default
loads basically instantaneously.
const LINKCHECK_EXCEPTIONS: &[(&str, &[&str])] = &[ | ||
// These are methods on slice, and `Self` does not work on primitive impls | ||
// in intra-doc links (intra-doc links are weird) | ||
// https://github.com/rust-lang/rust/issues/62834 is necessary to be |
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 meant the issue on primitive types.
// https://github.com/rust-lang/rust/issues/62834 is necessary to be | |
// https://github.com/rust-lang/rust/issues/63351 is necessary to be |
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.
oh, in my mind the generic issue is also what blocks slice, but your thing works
src/tools/linkchecker/main.rs
Outdated
// are cases where that does not work | ||
const LINKCHECK_EXCEPTIONS: &[(&str, &[&str])] = &[ | ||
// These are methods on slice, and `Self` does not work on primitive impls | ||
// in intra-doc links (intra-doc links are weird) |
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.
😆
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 actually meant to say "primitive impls are weird" lol
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
e9424e8
to
ec966ae
Compare
@bors r+ rollup=iffy The changes to linkchecker will mean any PR with broken links would break during rollup but not in a check build. |
📌 Commit ec966ae has been approved by |
…arth Rollup of 4 pull requests Successful merges: - rust-lang#74333 (Deny unsafe operations in unsafe functions in libstd/alloc.rs) - rust-lang#74356 (Remove combine function) - rust-lang#74419 (Add a thumbv4t-none-eabi target) - rust-lang#74485 (More intra-doc links, add explicit exception list to linkchecker) Failed merges: - rust-lang#74486 (Improve Read::read_exact documentation) r? @ghost
Fixes the broken links behind #32553
Progress on #32130 and #32129 except for a small number of links. Instead of whitelisting entire files, I've changed the code to whitelist specific links in specific files, and added a comment requesting people explain the reasons they add exceptions. I'm not sure if we should close those issues in favor of the already filed intra-doc link issues.