-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Rustdoc: generate unique ID attributes for each page #30036
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
Conversation
r? @cmr (rust_highfive has picked a reviewer for you, use r? to override) |
Uhm, I'm fairly certain that you can't use Why not use |
Does this also fix #25001? |
@killercup @apasel422 |
No, there isn't any such rule. Granted, it's slightly inconvenient to write |
I stand corrected. It still looks weird to me, though 😄 |
cc @alexcrichton |
Thanks for the PR @mitaa! This is looking pretty good to me, although I think it'd be better if the markdown document renderer supported just automatically handling this. It's a little unfortunate that you have to manually scope ids as it's easy to forget. I thought there was already support for this in the backend, but basically there should be a TLS map which stores all used ids and then every time you try to use one that's already used a number is just prepended to the end. Between pages this set can be reset, but the idea is that then this is always handled without having to manually annotate scopings. |
@alexcrichton I guess if this is (re)moved the issue would be solved, but I think this also has some disadvantages which I tried to solve with this approch:
How important are these aspects? |
I think it's ok to not have super-stable links, and I also think that the part about typing in makes more sense for major sections which are likely to have longer ids, consequently unique and easy-to-remember ones. |
Ah, alright, then I'll rewrite this PR to make the existing mechanism page global, (re)using it for all other ids on the page too. |
52eb35f
to
ef4b4b7
Compare
(updated) Should this include a test? edit: I think ID uniqueness should rather be enforced through |
USED_ID_MAP.with(|s| *s.borrow_mut() = init_ids()); | ||
} | ||
|
||
pub fn with_unique_id<T, F: FnOnce(&str) -> T>(candidate: String, f: F) -> T { |
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.
Can this return a unique id instead of taking a closure?
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.
Sure, I felt ambivalent about this anyway. (The intention was to avoid the extra heap allocation)
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.
Ah yeah don't worry too much about allocations, it happens a lot in rustdoc anyway :)
Looking good, thanks @mitaa! And yeah could you add a few small tests here and there in |
(updated) Does this look ok to you @alexcrichton? |
This expands the code which generates unique IDs for Markdown headers within a single block to each rendered page.
fixes #25001
fixes #29449