Skip to content

rejected edits trailing whitespaces #4687

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 21, 2025
+113 −19
Merged

rejected edits trailing whitespaces #4687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6fa09b
rejected edit trims the trailing whitespaces
panyamkeerthana Mar 17, 2025
3ba5452
remove redundant code
panyamkeerthana Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions core/diff/streamDiff.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,109 @@ describe("streamDiff(", () => {
await expectDiff("fastapi-tabs-vs-spaces.py");
});

test("trailing whitespaces should match ", async () => {
const oldLines = ["first item ", "second arg ", "third param "];

const newLines = ["first item", "second arg", "third param "];

const { streamDiffs } = await collectDiffs(oldLines, newLines);

expect(streamDiffs).toEqual([
{ type: "same", line: "first item " },
{ type: "same", line: "second arg " },
{ type: "same", line: "third param " },
]);
});

//indentation and whitespace handling
test.each([false, true])(
"ignores indentation changes for sufficiently long lines (trailingWhitespace: %s)",
async (trailingWhitespace) => {
let oldLines = [
" short",
" middle",
" a long enough line",
" short2",
"indented line",
"final line",
];

let newLines = [
"short",
"middle",
"a long enough line",
"short2",
" indented line",
"final line",
];

if (trailingWhitespace) {
oldLines = oldLines.map((line) => line + " ");
}

const { streamDiffs } = await collectDiffs(oldLines, newLines);
const expected = trailingWhitespace
? [
{ type: "old", line: " short " },
{ type: "new", line: "short" },
{ type: "old", line: " middle " },
{ type: "new", line: "middle" },
{ type: "same", line: " a long enough line " },
{ type: "same", line: " short2 " },
{ type: "same", line: "indented line " },
{ type: "same", line: "final line " },
]
: [
{ type: "old", line: " short" },
{ type: "new", line: "short" },
{ type: "old", line: " middle" },
{ type: "new", line: "middle" },
{ type: "same", line: " a long enough line" },
{ type: "same", line: " short2" },
{ type: "same", line: "indented line" },
{ type: "same", line: "final line" },
];

expect(streamDiffs).toEqual(expected);
},
);

test("preserves original lines for minor reindentation in simple block", async () => {
const oldLines = ["if (checkValueOf(x)) {", " doSomethingWith(x);", "}"];
const newLines = ["if (checkValueOf(x)) {", " doSomethingWith(x);", "}"];

const { streamDiffs } = await collectDiffs(oldLines, newLines);

expect(streamDiffs).toEqual([
{ type: "same", line: "if (checkValueOf(x)) {" },
{ type: "same", line: " doSomethingWith(x);" },
{ type: "same", line: "}" },
]);
});

test("uses new lines for nested reindentation changes", async () => {
const oldLines = ["if (checkValueOf(x)) {", " doSomethingWith(x);", "}"];
const newLines = [
"if (checkValueOf(x)) {",
" if (reallyCheckValueOf(x)) {",
" doSomethingElseWith(x);",
" }",
"}",
];

const { streamDiffs } = await collectDiffs(oldLines, newLines);

expect(streamDiffs).toEqual([
{ type: "same", line: "if (checkValueOf(x)) {" },
{ type: "new", line: " if (reallyCheckValueOf(x)) {" },
{ type: "old", line: " doSomethingWith(x);" },
{ type: "new", line: " doSomethingElseWith(x);" },
{ type: "old", line: "}" },
{ type: "new", line: " }" },
{ type: "new", line: "}" },
]);
});

test("FastAPI example", async () => {
await expectDiff("fastapi.py");
});
Expand Down
15 changes: 2 additions & 13 deletions core/diff/streamDiff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export async function* streamDiff(

let type: DiffLineType;

let isLineRemoval = false;
const isNewLine = matchIndex === -1;

if (isNewLine) {
Expand All @@ -45,7 +44,6 @@ export async function* streamDiff(
for (let i = 0; i < matchIndex; i++) {
yield { type: "old", line: oldLinesCopy.shift()! };
}

type = isPerfectMatch ? "same" : "old";
}

Expand All @@ -60,22 +58,13 @@ export async function* streamDiff(

case "old":
yield { type, line: oldLinesCopy.shift()! };

if (oldLinesCopy[0] !== newLine) {
yield { type: "new", line: newLine };
} else {
isLineRemoval = true;
}

yield { type: "new", line: newLine };
break;

default:
console.error(`Error streaming diff, unrecognized diff type: ${type}`);
}

if (!isLineRemoval) {
newLineResult = await newLines.next();
}
newLineResult = await newLines.next();
}

// Once at the edge, only one choice
Expand Down
11 changes: 8 additions & 3 deletions core/diff/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,23 @@ export function matchLine(
const isEndBracket = END_BRACKETS.includes(newLine.trim());

for (let i = 0; i < oldLines.length; i++) {
// trims trailing whitespaces from the lines before comparison
//this ensures trailing spaces don't affect matching.
const oldLineTrimmed = oldLines[i].trimEnd();
const newLineTrimmed = newLine.trimEnd();

// Don't match end bracket lines if too far away
if (i > 4 && isEndBracket) {
return { matchIndex: -1, isPerfectMatch: false, newLine };
}

if (linesMatchPerfectly(newLine, oldLines[i])) {
if (linesMatchPerfectly(newLineTrimmed, oldLineTrimmed)) {
return { matchIndex: i, isPerfectMatch: true, newLine };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this probably should be oldLines[i] not newLine? The goal here should be that when we return same - we actually have the old line, not a version that differs in trailing whitespace. First try to make a test case that fails, then change this.

Copy link
Contributor Author

@panyamkeerthana panyamkeerthana Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases I ran for this scenario passed. In streamDiff():

case "same":
    yield { type, line: oldLinesCopy.shift()! };
    break;

So, regardless of what matchLine() returns for newLine, the line that is actually output is the one removed from oldLinesCopy (which is the original old line with the trailing whitespaces). But I can change it to oldLine[i] to maintain logic and uniformity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned out that this very much needed to be newLine - the calling code relies on this only being different from the passed in newline when the leading indentation changed. Sorry for the confusion!

Copy link
Contributor Author

@panyamkeerthana panyamkeerthana Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this testing has only made the whitespace and indentation handling more robust!

}
if (linesMatch(newLine, oldLines[i], i)) {
if (linesMatch(newLineTrimmed, oldLineTrimmed, i)) {
// This is a way to fix indentation, but only for sufficiently long lines to avoid matching whitespace or short lines
if (
newLine.trimStart() === oldLines[i].trimStart() &&
newLineTrimmed.trimStart() === oldLineTrimmed.trimStart() &&
(permissiveAboutIndentation || newLine.trim().length > 8)
) {
return {
Expand Down
3 changes: 0 additions & 3 deletions core/edit/streamDiffLines.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,6 @@ export async function* streamDiffLines(
oldLines = [];
}

// Trim end of oldLines, otherwise we have trailing \r on every line for CRLF files
oldLines = oldLines.map((line) => line.trimEnd());

const prompt =
overridePrompt ??
constructPrompt(prefix, highlighted, suffix, llm, input, language);
Expand Down
Loading