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

Tracking issue for #[doc(spotlight)] #45040

Open
QuietMisdreavus opened this issue Oct 5, 2017 · 19 comments
Open

Tracking issue for #[doc(spotlight)] #45040

QuietMisdreavus opened this issue Oct 5, 2017 · 19 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@QuietMisdreavus
Copy link
Member

Initially implemented in #45039, this attribute puts your own trait on every function doc if its return type implements it.

@QuietMisdreavus QuietMisdreavus added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Oct 5, 2017
@jethrogb
Copy link
Contributor

Couple of observations:

  1. The exclamation point is to the left of the function signature, but it relates to the return type on the right. This can be confusing, especially if the trait implementations are not exactly the return type of the function (&[T] -> &[u8], see next comment).
  2. It seems pretty strange to me that https://doc.rust-lang.org/nightly/std/slice/struct.Iter.html#impl shows Read/Write for &[u8]. I'm probably not that interested in that.
  3. I think it would be more useful to me if I could force an ordering of impls on a doc page, so that the type author can decide what impls are relevant. For example, in https://docs.rs/hyper/0.10/hyper/client/request/struct.Request.html there's all this stuff which is mostly irrelevant and then all the way at the bottom you see theres impl Write which is what you actually need to put data in the request. (Although this particular example might just have been improved by adding a doc example at the top.)

@kennytm
Copy link
Member

kennytm commented Nov 22, 2017

I think it is more intuitive if the dialog is a popover that shows those "important traits" directly next to the ⓘ (mouseover will show it temporarily, click to make it stay).

screenshot_2017-11-23 01 56 46_el9bmr

@jonhoo
Copy link
Contributor

jonhoo commented Nov 22, 2017

I think it'd be handy to write up guidelines for when spotlight should be added to a trait. I had to search pretty deep through the history of this feature until I found out what the original use-case was. There's also another question of why this is a whitelist instead of a blacklist? Aren't trait implementations usually important, except those that are extremely common (like Clone, PartialEq, etc.)?

@ghost
Copy link

ghost commented Jan 8, 2018

The '🛈' sign being to the left of the function signature was confusing to me. I expected it to provide extra information about the function but it's actually related to the return type Iter<'a, T>. Perhaps putting it next to Iter<'a, T> would be better-aligned with its purpose?

My intuition is that the spotlight feature should really be just a simple balloon (tooltip) that shows up when hovering over '🛈' or Iter<'a, T>. So when you ask yourself "what's this return type all about?", you don't have to follow the link and scroll through the type's trait implementations, but instead you see a balloon that gives you a quick overview summarizing its trait implementations.

Maybe we don't even need the '🛈' sign - maybe it's enough to just show the balloon when hovering over the type. Also, clicking on '🛈' is a bit cumbersome - it feels like too much UI interaction for a quick peek into the return type. Finally, I'd probably prefer the balloon showing up when hovering over any occurence of the type Iter<'a, T> in the documentation, rather than in return type positions only.

@durka
Copy link
Contributor

durka commented Jan 8, 2018

FWIW I don't like the mouseover effect. It's surprising when I trigger it unintentionally, making things worse by covering the function signature I was trying to read. And I agree it should be somehow visually associated with the return type, instead of the current confusing location.

@QuietMisdreavus
Copy link
Member Author

@jonhoo The reason it was a whitelist was because i originally wanted this feature to be for "fundamental" traits, those that can effectively define an entire type. Things that implement Iterator are typically just there to be an Iterator. When i first got started on the implementation, i only put the spotlight attribute on Iterator, until i got a recommendation to also put it on Read and Write as well. The reason i didn't just put a static listing inside rustdoc was so that i could apply this to Future as well, since it has the same "fundamental" nature as Iterator.

However, putting every trait on there would blow out the window and possibly make it even worse than not having it there in the first place. Note that the idea came from #25928, and the fact that i wrote a generalized solution was mainly for Future, as mentioned earlier. (And also so i wouldn't have to make rustdoc specially dig out the Iterator trait, or some other static whitelist.)

@jethrogb Oh wow, i didn't even notice that &[T] -> &[u8] thing. I'll take a look at that and see what's up.


I initially wanted the circle-i icon to be next to the return type as well, but when @GuillaumeGomez put this UI together he said that we couldn't guarantee them appearing next to each other, in situations like line-wrapping and the like. There may have been issues getting everything together for the print as well. I wonder how much of a problem it would be to try to stick them next to each other.

@jonhoo
Copy link
Contributor

jonhoo commented Jan 12, 2018

@QuietMisdreavus ah, I see. I wonder if the name spotlight should be changed to better hint at that being the target use-case, rather than just "show this as special". Essentially, the name should communicate the semantic implications, more so than the desired visual outcome. It's too verbose, but something along the lines of #[doc(is_likely_primary_impl)]. Maybe #[doc(fundamental)] or #[doc(dominant)]? #[doc(notable)] also isn't terrible, but loses the "this is likely the primary behavior of implementors. An argument could also be made for #[doc(essential)], in that these traits are likely the essence of any implementing type, but unfortunately "essential" also carries may other connotations (along these lines, "constitutive" is great, but too obscure, and "intrinsic" is already used elsewhere).

Anyway, that's a lot of bikeshedding. I do think picking a more semantic name will lead to much less confusion for users though!

@durka
Copy link
Contributor

durka commented Jan 12, 2018 via email

@QuietMisdreavus
Copy link
Member Author

I like #[doc(fundamental)], or maybe #[doc(important)] to match the current text of the pop-up window. (Though i wonder whether we should rename the feature gate at the same time, and whether there's any kind of funny business that needs to happen in that case. >_>) The current #[doc(spotlight)] was just a random name that came to mind as i was dreaming up how the feature would be implemented, so i'm not that attached to it, heh.

@GuillaumeGomez
Copy link
Member

Maybe we should open a survey to pick a name?

@jonhoo
Copy link
Contributor

jonhoo commented Jan 12, 2018

I don't think renaming the feature should be too problematic. It's a nightly unstable feature for a reason! #[doc(important)] is problematic in the same way as #[doc(spotlight)] to me -- why is it important? Plenty of things are important (e.g., Sync, Clone, Debug, arguably most of the deriveable traits), but we don't want to highlight all of them. We want to highlight this one because we believe that the primary way you'll interact with an implementing type is through the marked trait. And the name should communicate that. #[doc(primary)] might even be a good fit, though "primary" already has so many other meanings...

@GuillaumeGomez I don't think it's outrageous to spend some time getting this name right before stabilization. I do agree with your sentiment that we shouldn't lament too much over this decision, but spotlight to me is too vague to be discoverable.

@GuillaumeGomez
Copy link
Member

Yes, that's why I suggested to open a survey. I don't have any personal preference on this topic and I have the feeling that's the case for everyone. So unless someone has a proposition, let's just open a survey or something along the lines. :)

@jonhoo
Copy link
Contributor

jonhoo commented Jan 13, 2018

@GuillaumeGomez ah, sorry, I thought you were being sarcastic! A survey seems a bit like overkill to me, but I'm not really opposed to it. How/where do you propose we do the survey?

@GuillaumeGomez
Copy link
Member

No, I was serious haha. And no idea. Either we open an issue where we put a few names and then people comment on it or we use a survey on a website and see what comes out. Once again, no preference at all on my side.

@TimDiekmann
Copy link
Member

Is it planned, that functions, which returns Result<T> or Option<T> also highlighted, when T implements a trait with #[doc(spotlight)]?

@jimblandy
Copy link
Contributor

@QuietMisdreavus wrote:

I initially wanted the circle-i icon to be next to the return type as well, but when @GuillaumeGomez put this UI together he said that we couldn't guarantee them appearing next to each other, in situations like line-wrapping and the like.

Couldn't you use a <span> styled with white-space: nowrap to keep the circle-i and the type together? MDN

@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Apr 23, 2019
@vincentdephily
Copy link
Contributor

It seems to me that it would be more obvious and flexible if the relationship went the other way: by tagging the impl instead of the trait.

  • Drawback: we will need to add the attribute in a lot of places.
  • Advantage: we can apply the tag more correctly. It's not hard to imagine a trait that is fundamental to one struct but nice-to-have to another.
  • Advantage: it would be more discoverable (see the spotlight icon, click [src], see the attribute).
  • Advantage: it can be generalized to many more usecases: showcase an impl/method/field of a struct/enum, but also the struct/enum within a module, or even a module within a crate.

@jyn514 jyn514 removed the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label Feb 11, 2021
@conradludgate
Copy link
Contributor

I was looking at the rustdocs the other day and I ended up re-inventing this attribute (I assumed rust-doc just had a hard coded list).

The version I came up with allowed the attribute to be in two different locations.

#[doc(notable)]
pub trait Iterator {}

#[doc(notable)]
impl Write for File {}

This solves two problems. First, it allows the original behaviour with how Iterators/Futures are always notable. And it solves another issue where Vec for some reason is notable for implementing write:

Screenshot from 2022-03-13 08-07-25

There's one more problem that I found which I'm not sure how to address just yet - In my opinion, File::open would display that File is notable for being Read/Write, but the function returns a Result so the hint doesn't take place

@conradludgate
Copy link
Contributor

A few more ideas:

There's one more problem that I found which I'm not sure how to address just yet - In my opinion, File::open would display that File is notable for being Read/Write, but the function returns a Result so the hint doesn't take place

We could somehow use the Try trait to our advantage? Determine if the return type implements Try, then use it's <Return as Try>::Output instead in the is_notable check.


Another point I wanted to make when I initially had my idea was that the documentation on the specific type's page does not indicate it's notable traits:

Screenshot from 2022-03-13 11-32-39

Instead, relying on the doc comments to point the user to the right place. This is fine but I feel like the hint would also be useful here if the reader found their way to this page.

Perhaps as well, we could use the foundations we have laid down from our Deref doc-block and have a separate Notable trait implementations block that ranks up in the page. For example:

Screenshot from 2022-03-13 11-40-07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests