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

Refactor internal suggestion API #45741

Merged
merged 2 commits into from
Nov 9, 2017
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 3, 2017

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2017
Copy link
Contributor

@estebank estebank left a 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.

&[(suggestion.msg.to_owned(), Style::NoStyle)],
max_line_num_len,
"suggestion",
Some(Style::HeaderMsg));
Copy link
Contributor

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();
Copy link
Contributor

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.

// 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();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 6, 2017

Changed PR so it makes no observable changes

@estebank
Copy link
Contributor

estebank commented Nov 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Nov 6, 2017

📌 Commit dfe218a has been approved by estebank

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017
@bors
Copy link
Contributor

bors commented Nov 9, 2017

⌛ Testing commit dfe218a with merge 5fe86f5eae8219fe3686b8556a85cf42c7219543...

@bors
Copy link
Contributor

bors commented Nov 9, 2017

💔 Test failed - status-travis

@estebank
Copy link
Contributor

estebank commented Nov 9, 2017

@bors retry

@bors
Copy link
Contributor

bors commented Nov 9, 2017

⌛ Testing commit dfe218a with merge a5067b6da8535c817900984f495130fe3842e5d9...

@bors
Copy link
Contributor

bors commented Nov 9, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Nov 9, 2017

@bors retry — travis-ci/travis-ci#8728

@bors
Copy link
Contributor

bors commented Nov 9, 2017

⌛ Testing commit dfe218a with merge 98e791e...

bors added a commit that referenced this pull request Nov 9, 2017
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)
@bors
Copy link
Contributor

bors commented Nov 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 98e791e to master...

@bors bors merged commit dfe218a into rust-lang:master Nov 9, 2017
@oli-obk oli-obk deleted the refactor_suggestions branch November 16, 2017 15:45
bors added a commit that referenced this pull request Nov 18, 2017
Remove left over dead code from suggestion diagnostic refactoring

More cleanups after #41876 and #45741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants