Improve rendering of crate features via doc(cfg) #75330

Nemo157 · 2020-08-09T12:24:23Z

The current rendering of crate features with doc(cfg(feature = "..")) is verbose and unwieldy for users, doc(cfg(target_feature = "..")) is special-cased to make it render nicely, and a similar rendering can be applied to doc(cfg(feature)) to make it easier for users to read.

I also added special casing of all/any cfgs consisting of just feature/target-feature to remove the repetitive "target/crate feature" prefix.

The downside of this current rendering is that there is no distinction between feature and target_feature in the shorthand display. IMO this is ok, or if anything target_feature should have a more verbose shorthand, because doc(cfg(feature = "..")) usage is going to vastly outstrip doc(cfg(target_feature = "..")) usage in non-stdlib crates when it eventually stabilizes (or even before that given the number of crates using cfg_attr(docsrs) like constructs).

Previously

Now

cc #43781

rust-highfive · 2020-08-09T12:24:27Z

r? @ollie27

(rust_highfive has picked a reviewer for you, use r? to override)

Dylan-DPC-zz · 2020-08-18T13:25:53Z

r? @GuillaumeGomez

GuillaumeGomez · 2020-08-18T13:29:36Z

I like the idea, however I think it needs some improvements: with this new wording, we don't know anymore that it's linked to features, which sounds problematic to me. However, I don't really have much ideas on how to make it obvious. Maybe adding #[cfg] mention somewhere?

cc @rust-lang/rustdoc

jyn514 · 2020-08-18T13:31:21Z

It does mention 'crate features' when you click on it, which seems pretty clear. Does it need to be on the index page too? It's already pretty cramped there.

GuillaumeGomez · 2020-08-18T13:34:00Z

It does mention 'crate features' when you click on it, which seems pretty clear. Does it need to be on the index page too? It's already pretty cramped there.

I was mostly referring to the second case, my bad. Currently it just shows some values with specific CSS but doesn't tell what it is.

Nemo157 · 2020-08-18T13:37:57Z

The whole "This is supported on" section is related to cfg. The old feature="foo" display seemed more confusing to me because that is related to the implementation syntax, not how the user declares that they need it (if we wanted to go away from the English description we could say something like "requires features = ["foo", "bar"]" to lean on the TOML usage syntax.

GuillaumeGomez · 2020-08-18T13:42:44Z

The old feature="foo" display seemed more confusing to me because that is related to the implementation syntax, not how the user declares that they need it

I strongly disagree here: it's "common" to have crates enabling features specifically on docs.rs so that the documentation is available in any case. So in my opinion, it's very important to make it clear to users that they need to use this feature if they want this item.

However, like I said, I don't like the current rendering and I think improving it is a good idea. However I don't agree with the changes you proposed. On the "short doc" views (in modules typically) to be exact.

Nemo157 · 2020-08-18T13:50:35Z

So in my opinion, it's very important to make it clear to users that they need to use this feature if they want this item.

We're very much not disagreeing then, I was at the forefront of the wave of cfg_attr(docsrs, doc(cfg(feature = "foo"))) usage; which is why I'm trying to make it more usable 😁.

For the shorthand, I think anything longer than this will be too long and more distracting than it's worth. As noted on the tracking issue:

Our index page becomes extremely noisy. I wish there were a way to not show all of these in our case.

@dtolnay it would be interesting to see if you still think so with this much more streamlined rendering?

Nemo157 · 2020-08-18T13:59:23Z

(I misspelled the mention first time, and not sure if github notifies based on edits).

@dtolnay do you think the rendering here is streamlined enough to want it displayed on the index pages, or would you still want to disable it in syn?

dtolnay

Thanks @Nemo157, I think this is reasonable and I would be okay with this on the index page for Syn. Once the second issue in #43781 (comment) is addressed we'll start using this.

Nemo157 · 2020-08-18T17:06:46Z

Once the second issue in #43781 (comment) is addressed we'll start using this.

(I am planning to look into this one too once this change is merged).

GuillaumeGomez · 2020-08-18T18:42:44Z

My concern still hasn't been addressed: docs are hard to read, removing all the information about it isn't a good idea I think.

jyn514 · 2020-08-18T18:48:50Z

My concern still hasn't been addressed: docs are hard to read, removing all the information about it isn't a good idea I think.

I still think it's fine to only have 'crate feature' on the item itself. You might not know the highlight is a feature the first time you read it on the index page, but it will be very clear after you click on the item and after that it will be better not to have the clutter.

Maybe we could also have it display a 'This is a crate feature' tooltip on hover or something?

GuillaumeGomez · 2020-08-18T18:53:27Z

Maybe we could also have it display a 'This is a crate feature' tooltip on hover or something?

That would be acceptable.

Nemo157 · 2020-08-18T19:19:12Z

Might be easier to have the full longform message in the tooltip?

GuillaumeGomez · 2020-08-18T20:24:13Z

I'm shared on having the long title in the HTML directly: it's increasing the HTML size once again. Not a blocker for me though, but makes me sad. Do you have a display when you hover it by any chance?

Nemo157 · 2020-08-18T20:26:30Z

Just had to work out how to take screenshots of tooltips 😁

jyn514 · 2020-08-18T20:29:59Z

src/test/rustdoc/duplicate-cfg.rs

+// @has 'foo/quux/index.html'
+// @matches '-' '//*[@class="module-item"]//*[@class="stab portability"]' '^sync and send and foo and bar$'
+// @has '-' '//*[@class="module-item"]//*[@class="stab portability"]/@title' 'This is supported on crate feature `sync` and crate feature `send` and `foo` and `bar` only'


nit: this would be nicer as sync, send, foo, and bar. But I think the existing behavior had this as well so not a blocker.

Nemo157 · 2020-08-18T20:43:40Z

it's increasing the HTML size once again.

This could be largely offset again by suppressing common cfgs from the current page and sub-items, that's what I plan to work on next (the linked comment only mentioned public fields, but I want to try prototyping it on all items first).

GuillaumeGomez · 2020-08-18T20:46:44Z

What do you mean by "common cfgs"?

Nemo157 · 2020-08-18T20:57:31Z

The last screenshot there is from a module which already requires the futures-io feature. So BrotliDecoder only really additionally requires the brotli feature active, so that's all it should show as required here (but then on the BrotliDecoder's page, since it's the top-level item, it should show the full set of cfg required to activate it).

Manishearth · 2020-08-19T04:42:44Z

I don't have strong opinions on this, the proposal looks fine but i'm excited to see if it can be improved. I'm going to unsubscribe while y'all discuss, feel free to tag me back in (or ping me elsewhere) if you want my opinion!

jyn514 · 2020-08-28T02:43:13Z

This seems like a great improvement to me. @GuillaumeGomez did you have any concerns unresolved? If not I think this is good to merge.

GuillaumeGomez · 2020-08-28T07:47:49Z

Well, I'm not a big fan but at least it's less verbose so let's go with it. Thanks @Nemo157 !

@bors: r+

bors · 2020-08-28T07:47:51Z

📌 Commit 3328bd9 has been approved by GuillaumeGomez

@ghost

…albini Rollup of 12 pull requests Successful merges: - rust-lang#75330 (Improve rendering of crate features via doc(cfg)) - rust-lang#75927 (Use intra-doc links in `core::macros`) - rust-lang#75941 (Clean up E0761 explanation) - rust-lang#75943 (Fix potential UB in align_offset doc examples) - rust-lang#75946 (Error use explicit intra-doc link and fix text) - rust-lang#75955 (Use intra-doc links in `core::future::future` and `core::num::dec2flt`) - rust-lang#75967 (Fix typo in `std::hint::black_box` docs) - rust-lang#75972 (Fix ICE due to carriage return w/ multibyte char) - rust-lang#75989 (Rename rustdoc/test -> rustdoc/doctest) - rust-lang#75996 (fix wording in release notes) - rust-lang#75998 (Add InstrProfilingPlatformFuchsia.c to profiler_builtins) - rust-lang#76000 (Adds --bless support to test/run-make-fulldeps) Failed merges: r? @ghost

Nemo157 added 2 commits August 9, 2020 14:19

Improve rendering of crate features via doc(cfg)

a8de713

Render longhand multiple crate/target features nicer

234ec95

rust-highfive assigned ollie27 Aug 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2020

Nemo157 mentioned this pull request Aug 9, 2020

Add doc attributes for docs.rs build Lokathor/tinyvec#105

Merged

jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 9, 2020

rust-highfive assigned GuillaumeGomez and unassigned ollie27 Aug 18, 2020

dtolnay reviewed Aug 18, 2020

View reviewed changes

jyn514 reviewed Aug 18, 2020

View reviewed changes

Add long cfg description to tooltip on short description

3328bd9

Nemo157 force-pushed the improve-doc-cfg-features branch from 4a13357 to 3328bd9 Compare August 18, 2020 20:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2020

pietroalbini mentioned this pull request Aug 28, 2020

Rollup of 12 pull requests #76019

Merged

bors merged commit 8730c2b into rust-lang:master Aug 28, 2020

Nemo157 mentioned this pull request Oct 7, 2020

Tracking issue for #[doc(cfg(…))], #[doc(cfg_hide(…))] and doc_auto_cfg #43781

Open

5 tasks

Nemo157 mentioned this pull request Nov 13, 2020

Add tests and improve rendering of cfgs on traits #78678

Merged

GuillaumeGomez mentioned this pull request Oct 1, 2022

Stabilize doc_cfg, auto_doc_cfg and cfg_hide features #100883

Closed

cuviper added this to the 1.48.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rendering of crate features via doc(cfg) #75330

Improve rendering of crate features via doc(cfg) #75330

Nemo157 commented Aug 9, 2020

rust-highfive commented Aug 9, 2020

Dylan-DPC-zz commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

jyn514 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020 •

edited

Loading

Nemo157 commented Aug 18, 2020

dtolnay left a comment

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

jyn514 commented Aug 18, 2020 •

edited

Loading

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

jyn514 Aug 18, 2020

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

Manishearth commented Aug 19, 2020

jyn514 commented Aug 28, 2020

GuillaumeGomez commented Aug 28, 2020

bors commented Aug 28, 2020

Improve rendering of crate features via doc(cfg) #75330

Improve rendering of crate features via doc(cfg) #75330

Conversation

Nemo157 commented Aug 9, 2020

Previously

Now

rust-highfive commented Aug 9, 2020

Dylan-DPC-zz commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

jyn514 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020 • edited Loading

Nemo157 commented Aug 18, 2020

dtolnay left a comment

Choose a reason for hiding this comment

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

jyn514 commented Aug 18, 2020 • edited Loading

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

jyn514 Aug 18, 2020

Choose a reason for hiding this comment

Nemo157 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Nemo157 commented Aug 18, 2020

Manishearth commented Aug 19, 2020

jyn514 commented Aug 28, 2020

GuillaumeGomez commented Aug 28, 2020

bors commented Aug 28, 2020

Nemo157 commented Aug 18, 2020 •

edited

Loading

jyn514 commented Aug 18, 2020 •

edited

Loading