-
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
Fix suggestions rendering when the diff span is multiline #107671
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
r? @estebank I hope you don't mind. It seems simpler, because we already discussed this in the issue |
Yes, in general, PRs that affect observable behavior should have a UI test associated with it. |
After adding more comments (particularly for presentation code, I find that having comments showing what each bit of code visually results in helps me a lot when I try to make changes 6 months later, and I always regret it when I don't find comments) and having some tests for these, I believe we can merge. |
I think this PR is mostly finished now. Maybe the test should be in another directory? I couldn't find one that seemed right. |
@bors r+ |
There is some follow up work here that I'd like to see. I've approved the PR. If you have time before bors merges it, update it and I'll re-approve. If not, lets make a follow up PR. |
Should be finished now. Thanks for all the help @estebank! |
@bors r=estebank |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5dd0e1b): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Fixes #92741
cc @estebank
I think, I finally fixed it. I still want to go back and try to clean up the code a bit. I'm open to suggestions.
Some examples of the new suggestions:
Should we add a test to ensure this behavior doesn't disappear in the future?