-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Add new "hide deprecated items" setting in rustdoc #151091
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
base: main
Are you sure you want to change the base?
Add new "hide deprecated items" setting in rustdoc #151091
Conversation
|
Some changes occurred in HTML/CSS/JS. |
This comment has been minimized.
This comment has been minimized.
1fe45dd to
4622f64
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
Rebased so it includes #151053, which hopefully should fix the flakyness issue. |
|
This made me realize that we use the word "hide" inconsistently in this settings panel, sometimes we mean "collapse, with a button to expand it again", and sometimes we mean "act like it doesn't exist at all". I think the former should be referred to with the verb "Collapse", I think that would make things seem a bit less inconsistent. |
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.
Logic looks sound, but there's a bunch of stuff that I think could be written either more readably or more compactly, and there's a few more test cases I would like to see.
src/librustdoc/html/render/mod.rs
Outdated
| Either::Right(boring) | ||
| }; | ||
|
|
||
| let mut deprecation_attr = if trait_item_deprecated || item.is_deprecated(cx.tcx()) { |
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.
| let mut deprecation_attr = if trait_item_deprecated || item.is_deprecated(cx.tcx()) { | |
| let mut deprecation_class = if trait_item_deprecated || item.is_deprecated(cx.tcx()) { |
This isn't a full attribute, only the value of the class attribute.
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.
This also makes it clear we aren't talking about the #[deprecation] (rust) attribute, but an html attribute.
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.
Good point, updating variable name then.
| } | ||
|
|
||
| fn item_module(cx: &Context<'_>, item: &clean::Item, items: &[clean::Item]) -> impl fmt::Display { | ||
| fn deprecation_class(is_deprecated: bool) -> &'static str { |
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.
| fn deprecation_class(is_deprecated: bool) -> &'static str { | |
| fn deprecation_class_attr(is_deprecated: bool) -> &'static str { |
This is a full attribute.
|
|
||
| let content = document_full(m, cx, HeadingOffset::H5).to_string(); | ||
|
|
||
| let mut deprecation_attr = |
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.
| let mut deprecation_attr = | |
| let mut deprecation_class = |
same nit as before, not a full attribute.
| if (getSettingValue("source-sidebar-show") === "true") { | ||
| addClass(document.documentElement, "src-sidebar-expanded"); | ||
| } | ||
| if (getSettingValue("hide-sidebar") === "true") { | ||
| addClass(document.documentElement, "hide-sidebar"); | ||
| } | ||
| if (getSettingValue("hide-toc") === "true") { | ||
| addClass(document.documentElement, "hide-toc"); | ||
| } | ||
| if (getSettingValue("hide-modnav") === "true") { | ||
| addClass(document.documentElement, "hide-modnav"); | ||
| } | ||
| if (getSettingValue("sans-serif-fonts") === "true") { | ||
| addClass(document.documentElement, "sans-serif"); | ||
| } | ||
| if (getSettingValue("word-wrap-source-code") === "true") { | ||
| addClass(document.documentElement, "word-wrap-source-code"); | ||
| } | ||
| if (getSettingValue("hide-deprecated-items") === "true") { | ||
| addClass(document.documentElement, "hide-deprecated-items"); | ||
| } |
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.
This is starting to look like a lot of duplicated code that could be made more compact by just calling into changeSetting, or by taking advantage of the fact most of the class names and the settings names are the same. Is there a reason this isn't done?
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.
No, I was thinking about it. Well, since you agree, I can simplify this code. :)
| case "hide-deprecated-items": | ||
| if (value === true) { | ||
| addClass(document.documentElement, "hide-deprecated-items"); | ||
| } else { | ||
| removeClass(document.documentElement, "hide-deprecated-items"); | ||
| } |
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 would think this could all be done much more compactly by having a single case body for all the settings that add/remove a class of the same name, no? like, addClass(document.documentElement, settingName); (and the same for removeClass) should handle the majority of settings, right?
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.
Same as previous comment. In short: strongly agree.
| #[deprecated(since = "1.26.0", note = "deprecated")] | ||
| pub struct DeprecatedStruct; | ||
|
|
||
| pub struct NonDeprecatedStruct; |
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.
we should have a since = "future" deprecation notice, and make sure that works correctly both in the regular module view and in rustdoc search (i'm not sure, but there's a chance future deprecations are actually treated differently between the two)
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 use is_in_effect, so it won't impact the display, adding a case though.
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.
My bad, it is impacted. ^^'
| // We disable the setting. | ||
| click: "#hide-deprecated-items" | ||
| // All of them should be displayed back. | ||
| wait-for-css: ("details.deprecated", {"display": "block"}, ALL) |
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.
just to make sure localStorage is working, one of these times we should keep the setting checked, go to a different page, make sure all the deprecated items are hidden, uncheck it, then make sure all the deprecated items are shown.
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.
Local storage is a bit tricky locally as it depends on the web browser whether or not the local storage is shared per-file or per-folder. So I'd rather not check that if you don't mind.
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, right. file://// is weird. one of these days it would be nice to also have some test ran against a normal http server, so we can test stuff like localstorage.
| // and so do file:/// URLs, which means we need to block | ||
| // it here if we want to avoid breaking the main docs site. | ||
| // https://github.com/rust-lang/rust/issues/145646 | ||
| block-network-request: "file://*//*" |
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.
why is this happening here an not literally anywhere else (genuine question)?
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 think it's because we try to load JS files which don't exist. As for the why, might be worth asking @notriddle and to update this comment.
|
@rustbot author |
…ng is working as expected
… setting, allowing to simplify JS code
4622f64 to
33bbf06
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 PR adds a new JS setting which allows the JS to hide deprecated items. This is especially useful for crates like
stdwhich accumulates deprecated items but never removes them.Nicely enough, the "deprecation" information was already in the search index, meaning I didn't need to change anything there. Before this PR, it was only used to make the deprecated items rank lower. Well now it's also used to generate an extra CSS class, allowing the JS setting to work.
This is taking over #149551.
This feature got approved by the rustdoc team in the meeting of december.
You can give it a try here.
r? @lolbinarycat