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

Fix broken links due to multiple mod paths for the same doc string #34463

Closed
wants to merge 3 commits into from

Conversation

JohnHeitmann
Copy link
Contributor

Fixes broken links for items documented at multiple module levels, like the DoubleEndedIterator links here:

https://doc.rust-lang.org/std/string/struct.String.html (broken currently)
vs.
https://doc.rust-lang.org/std/primitive.str.html (working)

This will completely break these links on doc sites hosted at non-root paths, but doc.rust-lang.org hosts at root.

r? @steveklabnik

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@TimNN
Copy link
Contributor

TimNN commented Jun 25, 2016

See #32129, #32553, #32130

Note that while this fix will work for the stable documentation, it will break the nightly & beta documentation.

@JohnHeitmann
Copy link
Contributor Author

Another way out would be to absolute link to doc.rust-lang.org. Would that be preferable to breaking nightly/beta/local?

@JohnHeitmann
Copy link
Contributor Author

A heavier-weight solution would be to modify rustdoc to process a token like $::$ into the appropriate amount of ../../. I know there's talk about revamping the markdown processor, but depending on the architecture of rustdoc it might fit better in a later processing stage. I'm going to be quiet here for a while while I look at how rustdoc works.

My concern isn't just about these few links. I'm trying to add a large amount more, so I'm willing to aim larger ammunition at the problem.

@steveklabnik
Copy link
Member

Absolute linking to the public site breaks local docs, so it's a no-go.

On Sun, Jun 26, 2016 at 3:50 PM, John Heitmann notifications@github.com
wrote:

A heavier-weight solution would be to modify rustdoc to process a token
like $::$ into the appropriate amount of ../../. I know there's talk
about revamping the markdown processor, but depending on the architecture
of rustdoc it might fit better in a later processing stage. I'm going to be
quiet here for a while while I look at how rustdoc works.

My concern isn't just about these few links. I'm trying to add a large
amount more, so I'm willing to aim larger ammunition at the problem.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34463 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AABsiiGYwrFoxXzw0UqIJdM482CkyZlgks5qPtfpgaJpZM4I-N0Q
.

@JohnHeitmann JohnHeitmann reopened this Jun 27, 2016
@JohnHeitmann
Copy link
Contributor Author

I added support to rustdoc for mod-root-relative links. Now you can write a link destination like ::/std/index.html and rustdoc will expand that with the appropriate level of ../s. This fixes the root of the problem, and makes it a bit easier to write links manually to boot.

It's future compatible with whatever link scheme we use with CommonMark extensions, since those should obviate markdown links entirely.

It isn't clear to me how public the Markdown and MarkdownToc types are. If they are very public, I'll need to undo the MarkdownToc breaking change. If they are entirely private to the rust repo I can clean up the construction API a bit.

Bikeshedding: the prefix could also be spelled mod:/ or mod::/ instead of ::/. mod:/ looks like a URL scheme, which is nice.

@JohnHeitmann
Copy link
Contributor Author

Testing notes: I couldn't figure out how to run the in-module test I wrote, so I ran it manually and it passes. I've also verified the result of make doc passes a full site dead link checker.

@TimNN
Copy link
Contributor

TimNN commented Jun 27, 2016

I believe all fixed pages should probably be removed from the link checkers whitelist at https://github.com/rust-lang/rust/blob/master/src/tools/linkchecker/main.rs#L125

@alexcrichton
Copy link
Member

I agree that we'll probably need to "extend markdown" somehow with some rustdoc-specific logic to solve this problem, but I'd be hesitant to do so as part of this PR without much discussion. We may want to move a little more slowly here and perhaps require an RFC for extending the syntax in rustdoc for generating links. I know there are quite a few opinions on this topic, so may want to ensure that there's broader agreement and more visibility into this!

@JohnHeitmann
Copy link
Contributor Author

TimNN: Ah! I didn't know there was a linkchecker built-in. I was running my own. The built-in linkchecker is catching more trouble than mine. I'll update both it and some doc strings with minor fixes.

alexcrichton: Yeah, I suspected an RFC might be called for. I was hoping to sneak this in to unblock my automatic linkification work, but I can see it needing a more noticeable discussion. I'm going to keep this open to capture one more round of bug fixes for posterity, then I'll close it out and work on the RFC.

@alexcrichton
Copy link
Member

@JohnHeitmann ok! It's fine to leave these links broken for now in non-std I think, though. We can certainly stomach a few small instances of broken links if there are bigger improvements in the pipeline.

@JohnHeitmann
Copy link
Contributor Author

Moving to an RFC: rust-lang/rfcs#1661

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.

5 participants