This repository was archived by the owner on Aug 31, 2023. It is now read-only.
fix(rome_diagnostics): improve the handling of newline characters in diffs #3523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
MichaReiser
approved these changes
Oct 28, 2022
|
|
||
| // Get all the lines | ||
| let mut parts = text.split('\n').enumerate(); | ||
| let mut parts = text.split('\n'); |
There was a problem hiding this comment.
Nit: lines? or is it guaranteed for code frames to never contain windows line breaks?
There was a problem hiding this comment.
It's possible for code frames or diffs to contain windows line breaks but they're handled separately in print_invisibles. The reason this code is using split instead of lines is because the later internally makes use of split_terminator instead and ignores the last line break, meaning "A\n".lines() would return ["A"] while "A\n".split('\n') returns ["A", ""]
| │ ----------- | ||
| 7 7 │ function test() { | ||
| 8 8 │ let a = 3; | ||
| 9 │ - ··debugger; |
There was a problem hiding this comment.
I like the new rendering. The additional context helps to understand which line was deleted.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3515
This PR changes how line breaks are handled in diffs, and specifically how individual lines get marked as "modified" when a line break is removed or inserted. It fixes an issue with the rendering of diffs from the formatter, but also changes when diffs are allowed to be rendered in short mode as the removal of an entire line is now shown in full mode (with the surrounding lines of context).
Test Plan
I've added an additional test case for this and updated the ellipsis case since it was actually being rendered incorrectly. I've also improved how markup assertion mismatches are displayed in the tests to ensure these kinds of issue are easier to notice in the future, and update a few snapshots related to the short mode changes.