Rustdoc label badge for notable traits#157058
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @lolbinarycat rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer Some changes occurred in compiler/rustc_hir/src/attrs |
|
r? @fmease rustbot has assigned @fmease. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
I would generated the color hash from the trait path (so |
|
|
||
| let Some(impls) = cx.cache().impls.get(&did) else { return Vec::new() }; | ||
|
|
||
| let mut out: Vec<LabelTraitInfo> = impls |
There was a problem hiding this comment.
Instead of using a Vec, would be better to use a BTreeMap. It's sorted on insert and prevents duplications.
There was a problem hiding this comment.
I changed to do that but then collect it to Vec, keeping this discussion opened as I'm not sure if you meant something else.
This comment has been minimized.
This comment has been minimized.
| // Evenly-spaced OKLCH hues at fixed light/chroma. | ||
| const BADGE_HUES: u64 = 8; | ||
| let hue = (h.finish() % BADGE_HUES) * 360 / BADGE_HUES; | ||
| let style_attr = format!("style=\"background: oklch(0.55 0.21 {hue})\""); |
There was a problem hiding this comment.
Please create CSS classes instead. We can iterate over the color of each class later on.
There was a problem hiding this comment.
By that I mean: you can put the current colors in the newly created CSS classes. There should be 6 colors from what we wrote in the rustdoc meeting, so 6 different CSS classes.
|
Reminder, once the PR becomes ready for a review, use |
993c2d1 to
a7dce51
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
a7dce51 to
bc59e96
Compare
This comment has been minimized.
This comment has been minimized.
bc59e96 to
bfb75d9
Compare
| name: String, | ||
| full_path: String, | ||
| /// Relative URL to the trait page, or `None` when not linkable. | ||
| href: Option<String>, |
There was a problem hiding this comment.
If the trait is not linkable, we should not generate a badge for it (I mentioned that here).
There was a problem hiding this comment.
By that I mean: you can put the current colors in the newly created CSS classes. There should be 6 colors from what we wrote in the rustdoc meeting, so 6 different CSS classes.
it does, yes ; we should probably decide what to do about the notable trait visibility on function return type to align behaviors ? I'd say keep it consistent and then unify behavior on a follow up pr ? But no strong feeling, can do here, I just fear controversial discussions on tangantial changes :p
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
98ddff6 to
cb6852b
Compare
This comment has been minimized.
This comment has been minimized.
cb6852b to
0886603
Compare
View all comments
its full pathShould the following be out of scope ?
#[doc(notable_trait(color="0xff0000")])Color discussion
I went for the higher chroma, even though some values use a fallback, I'm not opposed to go for a less chromatic palette, but that's very bikeshedding so I'll defer to authority.
with chroma 0.21:

with chroma 0.090:

History
Initially, this PR added a new feature "label_trait", it was then decided in rustdoc meeting to reuse current notable_trait feature.