feat: preserve EOL in files #16087
feat: preserve EOL in files #16087
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @Thomas-Shephard, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves file handling within the CLI tools by introducing robust line ending preservation. It ensures that file modifications respect the original line ending style (CRLF or LF) to prevent disruptive Git diffs, particularly for Windows users. Additionally, new files will now be created with the operating system's native line endings, and a previous bug that incorrectly stripped trailing newlines during content correction has been fixed, leading to more predictable and consistent file output. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to preserve existing line endings (CRLF/LF) in files modified by the WriteFile and Edit tools, which is a great improvement for cross-platform compatibility, especially on Windows. It also correctly defaults to the operating system's native line ending for new files and fixes a bug with trailing newlines during edit correction. The changes are well-structured, including moving the detectLineEnding function to a shared utility file and adding comprehensive regression tests. My review includes one suggestion to refactor a small piece of logic to improve maintainability by reducing code duplication.
| let finalContent = fileContent; | ||
| if (!isNewFile && originalContent) { | ||
| const lineEnding = detectLineEnding(originalContent); | ||
| if (lineEnding === '\r\n') { | ||
| finalContent = finalContent.replace(/\r?\n/g, '\r\n'); | ||
| } | ||
| } else { | ||
| if (os.EOL === '\r\n') { | ||
| finalContent = finalContent.replace(/\r?\n/g, '\r\n'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for handling line endings is correct, but it contains duplicated code which can make it harder to maintain. You can simplify this block by combining the conditions for converting to CRLF, which improves readability and maintainability.
let finalContent = fileContent;
const preserveCRLF = !isNewFile && originalContent && detectLineEnding(originalContent) === '\r\n';
// The `else` branch in the original logic covers new files or existing empty files.
const useOSEOLForNewFile = (isNewFile || !originalContent) && os.EOL === '\r\n';
if (preserveCRLF || useOSEOLForNewFile) {
finalContent = finalContent.replace(/\r?\n/g, '\r\n');
}
Thomas-Shephard
force-pushed
the
feature/eol-preservation
branch
from
da1ce7e to
3414bb7
Compare
There was a problem hiding this comment.
Code Review
This pull request is a great improvement for cross-platform compatibility by preserving file line endings (CRLF/LF). The changes in EditTool and WriteFileTool to respect existing EOLs and use the OS default for new files are well-implemented, and the addition of regression tests in line-endings.test.ts is excellent. I've found one issue in the new trimPreservingTrailingNewline helper function in editCorrector.ts which could lead to incorrect trimming of strings with trailing newlines followed by other whitespace. My review includes a suggestion to fix this.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
jacob314
added
the
status/need-issue
|
Hi @jacob314, I've now created an issue for this PR :) |
|
Hmmm
…On Wed, Jan 7, 2026 at 14:17 Thomas Shephard ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/core/src/utils/editCorrector.ts
<#16087 (comment)>
:
> + const trimmed = str.trim();
+ const match = str.match(/(\r?\n)+$/);
+ const trailingNewlines = match ? match[0] : '';
+ return trimmed + trailingNewlines;
Applied suggestion
—
Reply to this email directly, view it on GitHub
<#16087 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BVK5EFZ2R5DJTE6KQY6YS7D4FVSUJAVCNFSM6AAAAACQ7QJZKGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMZWGY2DINZRG4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
gemini-cli
bot
added
area/core
|
|
||
| const writtenContent = fs.readFileSync(filePath, 'utf8'); | ||
| // Expect 'line2' to be replaced by 'modified', and all endings to be CRLF | ||
| // Logic: |
There was a problem hiding this comment.
remove this comment block
|
Authored by /review-frontend, reviewed by Jacob Richman Overall: Great improvement for platform consistency and fixing the trailing newline bug! The use of os.EOL for new files is a solid addition. Consolidation Note: Please note that EditTool and SmartEditTool were recently consolidated into packages/core/src/tools/smart-edit.ts (commit f3625aa). You'll need to move your changes from edit.ts to smart-edit.ts. Deduplication: Now that detectLineEnding is in textUtils.ts, could you also update smart-edit.ts to use it and remove its local definition? Also, consider applying your improved /\r?\n/g regex there as well. Edit Corrector: The trimPreservingTrailingNewline fix looks correct. Tagging @anj-s to verify if this has any subtle impacts on LLM edit quality. Tests: In line-endings.test.ts, please use vi.restoreAllMocks() in afterEach for better test isolation. |
Thanks for the review! I checked the consolidation status, and it seems smart-edit.ts was renamed back to edit.ts recently, so I kept the changes there. I also verified that detectLineEnding is already being imported from textUtils in edit.ts and the local definition is removed, so that deduplication is taken care of. The improved regex is also already in place there. On the test side, I updated line-endings.test.ts to use vi.restoreAllMocks() in the afterEach block as requested, and I had to make a small tweak to write-file.test.ts to stop a restoreAllMocks flag from breaking the mock helpers. |
3 participants
Summary
This PR ensures that the
WriteFileandEdittools respect existing file line endings (CRLF vs LF) to prevent unnecessary Git diff noise, particularly on Windows. It also fixes a bug where trailing newlines were unintentionally stripped during edit correction and defaults to the operating system's native line ending (os.EOL) when creating new files.Details
As above
Related Issues
Relates to #3543
Closes #16148
Closes #16129
How to Validate
Run the new regression tests:
Verify that all tests pass, confirming CRLF preservation and OS EOL usage.
Run the updated edit corrector tests:
Verify that the new test case for trailing newlines passes.
Manual Verification (on Windows):
\r\n.Pre-Merge Checklist