Remove spotlight #69514
Remove spotlight #69514
Conversation
GuillaumeGomez
force-pushed
the
remove-spotlight
branch
from
2a11e7e
to
13c6d58
Compare
|
Formatting once again... |
|
I in fact do use it, though almost exclusively to figure out the item type of a returned iterator. I'm not sure I would consider such a poll conclusive evidence... On the other hand, I find clicking through to the relevant type not horrible, so maybe it's not so bad in practice to lose this. |
|
I think it's just too much information. If you want to check what "important trait" a type implements, just click on the type. The number of clicks remains the same but the amount of information is very different. But thanks for your feedback! :) |
|
Well, it is probably the same number of clicks, but I usually need to scroll back and forth to hunt down the iterator impl, whereas it's the "only" thing in the important traits usually. |
|
To be clear: I'm fine with this I think, just wanted to raise that we don't do a good enough job IMO about highlighting the item type of the iterator especially with this removed. I would almost want to highlight it directly (i.e., clickless), you almost always want it when you see an iterator in rustdoc. |
|
87 votes, people saying they never used it because they didn't know about it not sure if that's meaningful evidence for removing it |
|
I wonder (a) why one of the checks got stuck, and (b) whether as others have commented, this is the right reaction. In addition, this doesn't appear to significantly simplify things in the code, so I wonder if this does more than remove a small indicator and small piece of UX behaviour. What caused you to decide to hold this poll? |
|
Someone for the nth time asked me what what this "important traits" thing and what made those traits "important". I didn't like much this feature from the start since it's very arbitrary and doesn't provide much extra information: you just have to click on the type to see what's implemented on it. Also, the rustdoc output is already very heavy, this is removing a feature that not much people (to not say no one since I'm sure one or two people are using it...) are using. |
|
I think that justification is fair. As Mark said, it'd be useful to work out how to hilight the item type of an iterator (well, more generically, associated types for a trait) but that's not relevant for this change. I feel like @ForsakenHarmony has a good point that the poll is a very small sample, but then only two people reacted to the PR too. This change is moderately easy to back out if it comes to it, so I'm okay with it. If you can resolve whatever went wrong with the unfinished check (and re-trigger it if it's possible) then I'm OK to r+ this. |
|
Maybe @pietroalbini knows why or could restart it? |
|
Restarted it, seems like GitHub missed the update. |
JohnTitor
added
the
S-waiting-on-review
|
Then let's go! @bors: r=kinnison |
|
📌 Commit 13c6d58 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
…kinnison Remove spotlight I had a few comments saying that this feature was at best misunderstood or not even used so I decided to organize a poll about on [twitter](https://twitter.com/imperioworld_/status/1232769353503956994). After 87 votes, the result is very clear: it's not useful. Considering the amount of code we have just to run it, I think it's definitely worth it to remove it. r? @kinnison cc @ollie27
Rollup of 6 pull requests Successful merges: - #69475 (Remove the `no_force` query attribute) - #69514 (Remove spotlight) - #69677 (rustc_metadata: Give decoder access to whole crate store) - #69714 (Make PlaceRef take just one lifetime) - #69799 (Allow ZSTs in `AllocRef`) - #69836 (Check if output is immediate value) Failed merges: r? @ghost
Rollup of 10 pull requests Successful merges: - #69475 (Remove the `no_force` query attribute) - #69514 (Remove spotlight) - #69677 (rustc_metadata: Give decoder access to whole crate store) - #69714 (Make PlaceRef take just one lifetime) - #69799 (Allow ZSTs in `AllocRef`) - #69817 (test(patterns): add patterns feature tests to borrowck test suite) - #69836 (Check if output is immediate value) - #69847 (clean up E0393 explanation) - #69861 (Add note about localization to std::fmt docs) - #69877 (Vec::new is const stable in 1.39 not 1.32) Failed merges: r? @ghost
I had a few comments saying that this feature was at best misunderstood or not even used so I decided to organize a poll about on twitter. After 87 votes, the result is very clear: it's not useful. Considering the amount of code we have just to run it, I think it's definitely worth it to remove it.
r? @kinnison
cc @ollie27