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 · 15 comments

Comments

Projects
None yet
8 participants
@QuietMisdreavus
Member

QuietMisdreavus commented Oct 5, 2017

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

@jethrogb

This comment has been minimized.

Contributor

jethrogb commented Nov 22, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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.)?

@stjepang

This comment has been minimized.

Contributor

stjepang 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

This comment has been minimized.

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

This comment has been minimized.

Member

QuietMisdreavus commented Jan 12, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Contributor

durka commented Jan 12, 2018

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Jan 12, 2018

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

This comment has been minimized.

Member

GuillaumeGomez commented Jan 12, 2018

Maybe we should open a survey to pick a name?

@jonhoo

This comment has been minimized.

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

This comment has been minimized.

Member

GuillaumeGomez commented Jan 13, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Member

GuillaumeGomez commented Jan 13, 2018

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

This comment has been minimized.

Contributor

TimDiekmann commented Nov 29, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment