-
Notifications
You must be signed in to change notification settings - Fork 284
Implement side-by-side diffs #5925
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
Conversation
7cc0cc4
to
fa3e015
Compare
@@ -127,187 +127,436 @@ GET /api/projects/scratch/diff/terms?oldBranchRef=main&newBranchRef=new&oldTerm= | |||
RESPONSE: | |||
{ | |||
"diff": { |
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.
@hojberg note changes to the diff response.
Now the diff has {left: <lines>, right: <lines>}
where each side is an array of lines;
Each line is one of:
{kind: changed
, value: [<difftokens>]
}
{kind: unchanged
, value: [<difftokens>]
}
{kind: spacer}
The tokens themselves are similar to before, but for now each token is split up into its own object rather than having a both
with a list of tokens.
We may actually want to look at more efficient serializations since I know Mitchell had noticed the diff sizes were getting really big.
I'm also looking at IDs which associate tokens on the lhs with their rhs counter-part when they match; that way you could hover the lhs and have it light up the matching token on the other side if we want :)
@ChrisPenner how might I try this out with Share locally? |
@hojberg Here's a branch for ya! |
ddba7a2
to
49adb61
Compare
@aryairani this is already up on Share and seems to be working well :) Go ahead and merge unless you've got any concerns 👍🏼 |
-- diffSyntaxText :: SyntaxText -> SyntaxText -> [SemanticSyntaxDiff Syntax.Element] | ||
-- diffSyntaxText (AnnotatedText fromST) (AnnotatedText toST) = | ||
-- diffSegments syntaxElementDiffEq fromST toST | ||
-- & expandSpecialCases specialCaseAnnotations |
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.
still want this?
-- Helpers for testing semantic diffs on plaintext | ||
|
||
-- simpleLeft :: Text | ||
-- simpleLeft = "one word\ntwo words\nthree words" | ||
|
||
-- simpleRight :: Text | ||
-- simpleRight = "one word\ndifferent words\nthree words" | ||
|
||
-- complexLeft :: Text | ||
-- complexLeft = "one word\ntwo words\nthree words" | ||
|
||
-- complexRight :: Text | ||
-- complexRight = "one word\nmulti-line\ndifference\nshould add spacers\nthree words" |
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.
and these
cabal-version: 1.12 | ||
|
||
-- This file has been generated from package.yaml by hpack version 0.36.0. | ||
-- This file has been generated from package.yaml by hpack version 0.38.1. |
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 have a draft to have CI use the latest 'stack' but I haven't had a chance to test it. fingers crossed though
Overview
This represents a full overhaul of our diffing algorithm. The goal is to better align diffs in spite of change.
The old algorithm was entirely token-oriented, it didn't do anything to special-case lines, so if a bunch of lines were deleted on one side, the other side's diff would get out of alignment.
The new algorithm inserts spacers where necessary to keep similar chunks in line.
Here's an example diff using the new algorithm,
Each line is marked with -/+ when changed, and changed tokens within changed lines are highlighted by wrapping with
{}
(Don't worry, Simon will make the rendered version prettier)Implementation notes
At a high level, the algorithm:
This replaces the other diff algorithm entirely, which is currently unused in UCM, but will need matching updates from Simon when deploying to Share.
Test coverage
I added property tests to generate random diffs and assert that the diffs always have matching line-counts (asserting that spacers are being added)
I also added explicit test-cases using a text-rending of diffs.
There are also transcript tests to show the output json, but those aren't good for evaluating the diff effectiveness.
Loose ends
There's still another pass we can do on this to be smarter about where within a change block we put spacers, e.g. look at this VS code diff where spacers are interleaved within the change block to line up similar words.
Pretty sure I can iterate on this to get the same, but this is already a significant enough change that it's worth it to ship.