-
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
Restore linking to itself in implementors section of trait page #81313
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7d74be1
to
1668d3f
Compare
cc @sanxiyn, @nodakai, @steveklabnik The reason I think it makes sense to revert is because the change was only made for traits, which makes the documentation look inconsistent - some pages have links to themselves and others don't. I would be okay with a general feature which never adds a link if it goes to the current page, but it would be quite a bit more work and I don't think special-casing traits is the right approach in the meantime. @LeSeulArtichaut the implementation looks fine, but I'd prefer to hear back from one of the three people I pinged before merging this. |
I am fine with whatever the rustdoc team feels is appropriate here. |
☔ The latest upstream changes (presumably #80987) made this pull request unmergeable. Please resolve the merge conflicts. |
Steve said they're OK with whatever we (T-rustdoc) think. So: @rustbot label: -I-needs-decision Is an FCP necessary here? I would think not, but wanted to check. |
1668d3f
to
3e65258
Compare
This comment has been minimized.
This comment has been minimized.
3e65258
to
1daddb4
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.
I don't think this needs an FCP, no, this is just fixing an inconsistency rather than adding a feature.
use_absolute: bool, | ||
cache: &'a Cache, | ||
) -> impl fmt::Display + 'a { | ||
crate fn print<'a>(&'a self, cache: &'a Cache, use_absolute: bool) -> impl fmt::Display + 'a { |
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.
Is this ever called with use_absolute: true
? If not, can you remove the parameter?
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.
It's used here:
rust/src/librustdoc/html/render/mod.rs
Line 3773 in 1daddb4
write!(w, "{}", i.inner_impl().print(cx.cache(), use_absolute)); |
Where AFAICT use_absolute
can be true
from
rust/src/librustdoc/html/render/mod.rs
Lines 2476 to 2483 in 1daddb4
let use_absolute = match implementor.inner_impl().for_ { | |
clean::ResolvedPath { ref path, is_generic: false, .. } | |
| clean::BorrowedRef { | |
type_: box clean::ResolvedPath { ref path, is_generic: false, .. }, | |
.. | |
} => implementor_dups[&path.last()].1, | |
_ => false, | |
}; |
@bors r+ |
📌 Commit 1daddb4 has been approved by |
☀️ Test successful - checks-actions |
Reverts #32558 as proposed in this Zulip discussion
r? @jyn514 cc @camelid