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

Automatic Item linkification part 0 #34474

Closed
wants to merge 2 commits into from

Conversation

JohnHeitmann
Copy link
Contributor

@JohnHeitmann JohnHeitmann commented Jun 25, 2016

I wrote a script to automatically create reference links for item names. It covers macros, structs, traits, modules, enums, constants, and module-level functions. It does not handle methods or enum variants. In ambiguous situations like Iter it does nothing. However, it has special-case disambiguations for the core -> std duplication, and will always disambiguate to std.

Foo<Bar<T>> links entirely to Foo, not partially to Foo and partially to Bar (this is fixable if it's a problem).

I've used a broken link checker to automatically verify all links. Also, the script does not synthesize reference destinations from scratch, instead it scrapes them from an already-built doc/ tree, so there's an additional layer of safety.

The full, huge, change is here: JohnHeitmann@249dabe

In this first merge I've tried to highlight interesting quirks.

I had to use root-relative links for libcollections since those items are documented at multiple module levels. libstd and libcore both use true relative links.

The things I'm least happy with currently are places like BTreeMap below where a type gets linked to itself. This is undesirable noise. I can't perfectly fix it since I'm just running dumb regular expressions to parse, but I should be able to add some heuristics to get rid of most instances. Also, I haven't looked into why panic! isn't linked. My std/core disambiguation might be partially broken there.

So, I still have some polishing to do, but I wanted to get this out for feedback in case this was broadly unacceptable for some reason.

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.

@steveklabnik
Copy link
Member

No, this is super neat! Thanks a ton. Such a thing sounds extremely useful, please send more!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jun 25, 2016

📌 Commit 0149b9f has been approved by steveklabnik

@JohnHeitmann
Copy link
Contributor Author

I updated these isolated examples after some script fixes. Now docs for items won't reference the item. Docs for members of items will still reference the parent (because it's pretty hard to implement that kind of suppression, not because I think it's the best solution).

I also fixed some whitespace detection bugs causing missed links for nested member docs.

I looked at the missing macro link bug. It's a design problem, so it's tricky to fix. Some macros are linking, others aren't. I'll hack up a workaround later once the main changes are done.

I'm happy with where this PR is now and it's ready to be pulled. Once this goes through I'll start sending PRs containing batches of 10 to 20 files (or whatever is easiest for you).

@ollie27
Copy link
Member

ollie27 commented Jun 26, 2016

The root-relative links aren't going to work in most places like https://doc.rust-lang.org/nightly/std/ and when viewing the docs offline so we can't use those.

@@ -33,6 +33,8 @@
/// panic!(4); // panic with the value of 4 to be collected elsewhere
/// panic!("this is a {} {message}", "fancy", message = "message");
/// ```
///
/// [`Box<Any>`]: ../../collections/boxed/struct.Box.html
Copy link
Member

Choose a reason for hiding this comment

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

There is one too many ../s for the links you've added in this file. The macros are documented at the crate root so you only need to go up once.

Also this one should really be linking to std::boxed::Box and not collections::boxed::Box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catches, thanks. My web server was eating the extra '../' so my dead link checker didn't catch that.

Is there any situation where a type is mirrored into std that I shouldn't treat the std version as canonical? e.g. if something in collections mentions Box is it ok for that link to be to std, or will I need custom resolution per lib?

Copy link
Member

Choose a reason for hiding this comment

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

I suggest you look into linkchecker.

In my opinion the std docs are most important so as long as the links in that stay within std it shouldn't matter too much about the other crates.

@JohnHeitmann
Copy link
Contributor Author

See here for context on the root-relative links: #34463

@JohnHeitmann
Copy link
Contributor Author

My script had entirely broken ../ handling. Fixing it looks tricky. Postponing this while I try to tackle the relative link problem: 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