-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove highlighting from secondary messages #51485
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
I'm sure this might be controversial. There was a ticket asking for this that I couldn't find now. I'm looking for feedback. cc @nikomatsakis @GuillaumeGomez @jonathandturner @petrochenkov |
Can you post the original look too please? So we can see the differences more easily. |
This comment has been minimized.
This comment has been minimized.
Deemphasize the secondary messages so that all other highlights stand out more.
f83f271
to
ed5dcc3
Compare
I don't have strong opinion in here. It allows to focus more easily on the other messages, which depending on what we want might be better or worse. So do we want to focus on help messages, attract users' attention to tips or do we want to keep the attention on the first/initial/main error? |
My reasoning is that we're already using span_notes and notes as part of the same "narrative", while on the cli output we give span_notes a highlight but not to notes, incorrectly differentiating them. I think it is a bikeshed-ing argument in any direction, but I'd like us to be consistent.
For what is worth, many vocal users want less highlighting and to focus on the main message. The proposed output would train people that bold white is where to look for the main message. |
I have no strong opinions one way or another. |
I'd just like to have at least @nikomatsakis or @jonathandturner's opinion before r+ing it. |
Personally, I think this is an improvement. |
While I think the general rule of thumb to decrease the amount of bold is probably a good thing, to my eyes, this is a small step backward. Specifically, this problem spot seems to have gotten worse: In the first version, it's easy for the eye to separate out the first message from the second. In the revision, these visually merge together, which takes a little longer to parse at first glance. One way to work around this might be to remove the bold, but add in a blank line. I think @nikomatsakis and I settled on using bold so that we wouldn't use too much spacing out of lines, but perhaps it's worth exploring again? |
The thing is that those three lines are actually part of the same narrative, so it doesn't seem right to differentiate them as highly as we currently are. |
Ping from triage! @nikomatsakis can you check in on this? |
@nikomatsakis could you take a look at this? The diff is minimal, and the output change can be seen in the screenshots above. The only remaining argument remaining is @jonathandturner's worry that multiple notes one after the other will be harder to read. I don't think that is the case, and that the current unnecessary differentiation between notes with spans and without them is more detrimental to readability than this PRs output. |
I like it |
I agree with @jonathandturner that this particular case is slightly worse but I also think those messages are bad and something we are trying to revamp in a major way for NLL. =) |
@GuillaumeGomez what do you think? Should we merge this as is? |
Like I said, it doesn't make much differences for me. I gave the two opinions I had on the question but both are fine for me. :) |
Ping from triage @estebank , we haven't heard from you, will you have time to look into this PR, in near future? |
@stokhos I don't see what's left for me to do on the PR.
That is something to be done on a case by case basis.
I believe that we need to make it easier to read the errors. There are people that are driven towards the error message and others driven towards the labels. With this change we are removing the attention seeking highlight from the notes, which should only be used to further understand the error, not the entry point. |
Ping from triage! @GuillaumeGomez this PR needs to be moved forward. |
📌 Commit ed5dcc3 has been approved by |
⌛ Testing commit ed5dcc3 with merge d439d6cde2ca0afe1d6f56e2e494cd1b35f3d7a5... |
💔 Test failed - status-appveyor |
@bors retry |
…, r=GuillaumeGomez Remove highlighting from secondary messages Deemphasize the secondary messages so that all other highlights stand out more. <img width="684" alt="" src="https://user-images.githubusercontent.com/1606434/41261199-7b4fe96e-6d8f-11e8-8619-04d170617df2.png">
…meGomez Remove highlighting from secondary messages Deemphasize the secondary messages so that all other highlights stand out more. <img width="684" alt="" src="https://user-images.githubusercontent.com/1606434/41261199-7b4fe96e-6d8f-11e8-8619-04d170617df2.png">
☀️ Test successful - status-appveyor, status-travis |
Deemphasize the secondary messages so that all other highlights stand
out more.