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

[rustdoc] Implementors section of Sync (and other similar traits) should separate implementors and !implementors #51129

Open
alex opened this issue May 28, 2018 · 7 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@alex
Copy link
Member

alex commented May 28, 2018

image

The !Sync implementations and the Sync implementations really ought to be separated apart, otherwise it makes it harder to skim.

https://doc.rust-lang.org/stable/std/marker/trait.Sync.html#implementors

@QuietMisdreavus QuietMisdreavus added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jun 14, 2018
@QuietMisdreavus
Copy link
Member

Would it work out if the impls were just sorted apart, rather than giving an entirely new section heading to "negative impls"? That might be easier to implement.

@alex
Copy link
Member Author

alex commented Jun 14, 2018

@QuietMisdreavus yes, just sorted seperately would be A+

@QuietMisdreavus
Copy link
Member

Excellent!

Some mentoring instructions:

The code that displays all these impls is right here:

let (local, foreign) = implementors.iter()
.partition::<Vec<_>, _>(|i| i.inner_impl().for_.def_id()
.map_or(true, |d| cache.paths.contains_key(&d)));
let (synthetic, concrete) = local.iter()
.partition::<Vec<_>, _>(|i| i.inner_impl().synthetic);
if !foreign.is_empty() {
write!(w, "
<h2 id='foreign-impls' class='small-section-header'>
Implementations on Foreign Types<a href='#foreign-impls' class='anchor'></a>
</h2>
")?;
for implementor in foreign {
let assoc_link = AssocItemLink::GotoSource(
implementor.impl_item.def_id, &implementor.inner_impl().provided_trait_methods
);
render_impl(w, cx, &implementor, assoc_link,
RenderMode::Normal, implementor.impl_item.stable_since(), false)?;
}
}
write!(w, "{}", impl_header)?;
for implementor in concrete {
render_implementor(cx, implementor, w, &implementor_dups)?;
}
write!(w, "</ul>")?;
if t.auto {
write!(w, "{}", synthetic_impl_header)?;
for implementor in synthetic {
synthetic_types.extend(
collect_paths_for_type(implementor.inner_impl().for_.clone())
);
render_implementor(cx, implementor, w, &implementor_dups)?;
}
write!(w, "</ul>")?;
}

All the collections it's partitioning at the start of that contain render::Impl structs, and you can see whether it's a !impl by checking the "polarity" of the clean::Impl it contains. You can check implementor.impl_item().polarity and compare it against Some(clean::ImplPolarity::Negative), and sort these collections before iterating (or partition them farther, heh) to print one before the other. Given the nature of Send and Sync, i think it would be better to print the negative impls before the positive ones.

@QuietMisdreavus QuietMisdreavus added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jun 14, 2018
@SLaabsDev
Copy link

I can start on this one

@QuietMisdreavus
Copy link
Member

@SLaabsDev Thanks! Were the instructions i gave a good start? If you need any help, please ask me!

@SLaabsDev
Copy link

SLaabsDev commented Jun 26, 2018

@QuietMisdreavus Yes, the instructions were very helpful! I have it to a point where it is sorting with negative implementors showing first. One question I have is outside of the polarity does the order of implementors need to be preserved?

@QuietMisdreavus
Copy link
Member

@SLaabsDev That's probably best addressed by all of @rust-lang/docs but what i would say is "don't worry about it", since we currently don't do a very good job of keeping that ordering in the first place. This could be a good place to enforce a good ordering, but you don't have to do that in your PR. (If you asking to choose between sort and sort_unstable, go ahead and pick sort just so we don't disrupt the current ordering too much.)

@camelid camelid self-assigned this Nov 26, 2020
@camelid camelid changed the title [docs] Implementors section of Sync (and other similar traits) should separate implementors and !implementors [rustdoc] Implementors section of Sync (and other similar traits) should separate implementors and !implementors Nov 26, 2020
@camelid camelid removed their assignment Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
4 participants