-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve trailing newlines in stripLineNumbers for apply_diff (#8020) #8021
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
- Modified stripLineNumbers function to preserve original trailing newlines - This fixes issue #8020 where line numbers at the last line were not being stripped correctly - The regex in multi-search-replace.ts was removing trailing newlines, causing stripLineNumbers to fail - Added tests to verify the fix works for various scenarios including Windows CRLF line endings
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.
Reviewing my own code is like debugging in a mirror - everything looks backward but the bugs are still mine.
| }) | ||
|
|
||
| // Join back with original line endings (carriage return (\r) + line feed (\n) or just line feed (\n)) | ||
| const lineEnding = content.includes("\r\n") ? "\r\n" : "\n" |
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.
Is this approach intentional? The function detects line endings by checking if ANY '\r\n' exists in the content. This could cause issues with mixed line endings - for example, if a file has mostly LF endings with just one CRLF, it would treat everything as CRLF. Would it be better to check which line ending is more prevalent, or handle mixed endings differently?
| const result = processedLines.join(lineEnding) | ||
|
|
||
| // Preserve the original trailing newline if it existed | ||
| // This is important for diffs where the last line might not have a newline |
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.
Nice fix for preserving trailing newlines! Consider enhancing this comment to explicitly mention that this addresses the issue where the regex in multi-search-replace.ts removes trailing newlines with (?:\n)?. This would help future maintainers understand the context better.
| + CollectionUtils.size(idNoAddressInfoList) + CollectionUtils.size(workAddressInfoList) | ||
| + CollectionUtils.size(personIdentityInfoList));` | ||
| expect(result.content).toBe(expectedContent) | ||
| } |
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.
Good test coverage for CRLF line endings! Could we also add a test case with mixed line endings (both CRLF and LF in the same content) to ensure the function handles this edge case correctly? This would catch the potential issue I mentioned about line ending detection.
|
The root cause remains in the parser regex in The pattern still optionally consumes the trailing newline for the SEARCH/REPLACE captures (via the optional Action: adjust the regex so the trailing newline is not consumed (keep it outside the capture via lookahead, or explicitly capture and re-emit), especially around the For example, change the segment that ends the SEARCH capture from: ([\s\S]?)(?:\n)?(?:(?<=\n)(?<!)=======\s\n)to: ([\s\S]?)(?=(?<=\n)(?<!)=======\s\n)and do the analogous change for the REPLACE block. This keeps |
Description
This PR fixes issue #8020 where line numbers at the last line were not being stripped correctly when using apply_diff with SEARCH/REPLACE blocks.
Problem
The regex pattern in multi-search-replace.ts was capturing content with optional trailing newlines using
(?:\n)?, which removed the trailing newline before passing content to stripLineNumbers(). This caused the stripLineNumbers function to fail on the last line because it couldn't match the line number pattern without a proper line ending.Solution
Modified the
stripLineNumbersfunction insrc/integrations/misc/extract-text.tsto preserve the original trailing newlines. The function now:Testing
Related Issue
Fixes #8020
Checklist
Important
Fixes trailing newline preservation in
stripLineNumbersinextract-text.tsand adds comprehensive tests.stripLineNumbersinextract-text.tsto preserve trailing newlines.multi-search-replace-issue-8020.spec.ts,multi-search-replace-line-number.spec.ts, andmulti-search-replace-simple.spec.ts.This description was created by
for bd27154. You can customize this summary. It will automatically update as commits are pushed.