-
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
Refactor internal suggestion API #45741
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM, intrigued about the reason to highlight trailing whitespace as well, as to my eyes it doesn't improve understanding but makes it slightly uglier.
src/librustc_errors/emitter.rs
Outdated
&[(suggestion.msg.to_owned(), Style::NoStyle)], | ||
max_line_num_len, | ||
"suggestion", | ||
Some(Style::HeaderMsg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realign to opening paren.
// entirety of the code being shown and the displayed code is not multiline. | ||
if show_underline { | ||
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1); | ||
let sub_len = sub.trim_right().len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To other reviewers, this line's change is the one that causes the output change.
src/librustc_errors/emitter.rs
Outdated
// entirety of the code being shown and the displayed code is not multiline. | ||
if show_underline { | ||
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1); | ||
let sub_len = parts[0].snippet.len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to include trailing whitespace in the suggestions? I feel it looks slightly cleaner when looking at it in the terminal as it is pointing at the code you need to write only, while it is still there for the RLS to suggest the properly formatted code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was less correct, but you're right, there's no gain in knowing about the whitespace in the terminal output. I'll fix it.
Changed PR so it makes no observable changes |
@bors r+ |
📌 Commit dfe218a has been approved by |
⌛ Testing commit dfe218a with merge 5fe86f5eae8219fe3686b8556a85cf42c7219543... |
💔 Test failed - status-travis |
@bors retry |
⌛ Testing commit dfe218a with merge a5067b6da8535c817900984f495130fe3842e5d9... |
💔 Test failed - status-travis |
@bors retry — travis-ci/travis-ci#8728 |
Refactor internal suggestion API ~~The only functional change is that whitespace, which is suggested to be added, also gets `^^^^` under it. An example is shown in the tests (the only test that changed).~~ Continuation of #41876 r? @nagisa the changes are probably best viewed [without whitespace](https://github.com/rust-lang/rust/pull/45741/files?w=1)
☀️ Test successful - status-appveyor, status-travis |
The only functional change is that whitespace, which is suggested to be added, also gets^^^^
under it. An example is shown in the tests (the only test that changed).Continuation of #41876
r? @nagisa
the changes are probably best viewed without whitespace