-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
rustdoc: Merge method
and tymethod
URL fragments and sections
#92658
Conversation
Some changes occurred in HTML/CSS/JS. |
This comment has been minimized.
This comment has been minimized.
7ef0ed1
to
c996ec3
Compare
I think the main downside of this change is the breakage of non–intra-doc links. There are a lot of results when I search for Should we have an FCP given the breakage? |
Why not add JS to fixup tymethod fragments on page load? |
I guess we could... I just don't want to end up with tons of redirects as we change rustdoc's output over time. Is there existing code for this or should I add it? |
This comment has been minimized.
This comment has been minimized.
I'd rather not have JS code for that... Such things tend to last much longer than expected. Considering how big the impact is and because it's also a breaking change, FCP is required here. |
Hmm, I'm sympathetic to the concern about the redirect lasting too long, but on the other hand, it shouldn't be much code to maintain and the change would otherwise be annoying for users. |
The code would be something like (in main.js): document.addEventListener("DOMContentLoaded", function() {
let fragment = getPageId(); // getPageId is defined in main.js
if (fragment.startsWith("tymethod.")) {
document.location.hash = "#" + fragment.slice(2);
}
} |
This comment has been minimized.
This comment has been minimized.
This change was discussed on Zulip [here][1]. Here is an excerpt of my proposal, explaining my rationale: They add needless complexity to the intra-doc links code (e.g., [whether an associated item is an impl vs a prototype has to be hardcoded in some circumstances][2]) and elsewhere in rustdoc. I don't think there's any need to distinguish between these two kinds. For example, associated constant prototypes in traits vs definitions in traits, trait impls, and inherent impls both use `associatedconstant`. [1]: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/.5Bproposal.5D.20merge.20tymethod.20and.20method [2]: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/trait.20assoc.20item.20-.3E.20impl.20assoc.20item
This commit replaces all uses of `tymethod` with `method` in rust-lang/rust, including subtrees but not submodules. The Book, Nomicon, and Reference will have to be updated separately after this change lands.
804c509
to
889d3f9
Compare
I moved the CSS cleanup commit into its own PR: #93444 |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #93655) made this pull request unmergeable. Please resolve the merge conflicts. |
…llaumeGomez Refactor sidebar printing code This is the refactoring parts of rust-lang#92660, plus the trait aliases capitalization consistency fix. I think this will be necessary for rust-lang#92658. r? `@GuillaumeGomez`
Ping from triage: |
I've been meaning to get back to it. Hopefully within the next few weeks I'll be able to finish it. Otherwise, I'll probably close it. |
Closing this as inactive |
This change was discussed on Zulip here.
Here is an excerpt of my proposal, explaining my rationale:
r? @GuillaumeGomez
cc @jsha