fix(psl): preserve schema line endings during reformat#5814
Open
barobaonguyen wants to merge 1 commit into
Open
fix(psl): preserve schema line endings during reformat#5814barobaonguyen wants to merge 1 commit into
barobaonguyen wants to merge 1 commit into
Conversation
`prisma format` previously emitted LF for every line of its output but ended the file with a CRLF when the input used CRLF anywhere. That left Windows users with a stray `\r\n` on the last line of an otherwise-LF file (or vice versa) and broke editors that disallow mixed line endings. Fix it natively in the renderer instead of post-processing the output (the maintainer rejected post-processing in prisma#5812). The reformatter now: * Detects the input's line-ending style (first newline wins; mixed inputs fall back to LF) via a new `NewlineType::detect` helper. * Threads the chosen ending through `Renderer` so each `end_line` writes the configured separator. The trailing-newline guard in `reformat` is updated to check for and append the configured ending too. * Exposes a `reformat_with_line_ending` entry point alongside the existing `reformat` for callers that want to pin a specific style. Closes prisma/prisma#8548
|
|
Summary by CodeRabbitRelease Notes
WalkthroughThis PR implements line-ending preservation in schema reformatting. The formatter now detects whether input uses LF or CRLF line endings and preserves that style in the output, with mixed line endings defaulting to LF. A regression test ensures the output correctly terminates with the detected line ending. ChangesLine-ending preservation in schema reformatting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the PSL reformatter to preserve (or explicitly control) output line endings (LF vs CRLF), addressing a regression where output could end with a different newline style than the body.
Changes:
- Add
NewlineType::detect()and threadNewlineTypethroughRendererso all emitted newlines are consistent. - Introduce
reformat_with_line_ending()and havereformat()auto-detect the input’s line-ending style. - Add regression tests ensuring LF/CRLF consistency and correct trailing newline behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| psl/schema-ast/src/renderer.rs | Renderer now emits newlines using a configured NewlineType instead of always \n. |
| psl/schema-ast/src/reformat.rs | Adds auto-detection and a public API to force line endings; ensures trailing newline matches chosen ending. |
| psl/schema-ast/src/lib.rs | Re-exports reformat_with_line_ending as part of the public API surface. |
| psl/schema-ast/src/ast/newline_type.rs | Adds NewlineType::detect() to infer newline style from an input string. |
| psl/psl/tests/reformat/reformat.rs | Adds regression tests for line-ending preservation and consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+36
to
+47
| pub fn detect(input: &str) -> NewlineType { | ||
| let bytes = input.as_bytes(); | ||
| for (i, &b) in bytes.iter().enumerate() { | ||
| if b == b'\n' { | ||
| if i > 0 && bytes[i - 1] == b'\r' { | ||
| return NewlineType::Windows; | ||
| } | ||
| return NewlineType::Unix; | ||
| } | ||
| } | ||
| NewlineType::Unix | ||
| } |
Comment on lines
+13
to
+14
| /// The output preserves the line-ending style detected from `input` (LF or | ||
| /// CRLF). Mixed-ending inputs fall back to LF. See `NewlineType::detect`. |
Comment on lines
+1284
to
+1294
| #[test] | ||
| fn mixed_input_defaults_to_lf() { | ||
| // First line uses LF, later lines use CRLF: the first newline wins | ||
| // and the reformatter falls back to LF for the whole output. | ||
| let input = format!( | ||
| "{}\n{}\r\n{}\r\n{}\r\n", | ||
| SCHEMA_LINES[0], SCHEMA_LINES[1], SCHEMA_LINES[2], SCHEMA_LINES[3] | ||
| ); | ||
| let out = reformat(&input); | ||
| assert!(!out.contains('\r'), "mixed input should normalize to LF: {out:?}"); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@psl/psl/tests/reformat/reformat.rs`:
- Around line 1285-1294: Add a second test variant that mirrors
mixed_input_defaults_to_lf but with CRLF on the first line(s) and LF later to
ensure normalization behavior is symmetric; duplicate the test logic in a new
function (e.g., mixed_input_with_crlf_first) using the same SCHEMA_LINES
sequence but build input so the first newline is "\r\n" and later lines use "\n"
(or construct format with CRLF-first then LF), call reformat(&input) and assert
no '\r' in out exactly like the original test to lock the mixed-ending contract
from both directions.
In `@psl/schema-ast/src/ast/newline_type.rs`:
- Around line 36-47: The detect function currently returns on the first newline
and misclassifies mixed CRLF+LF input as Windows; update NewlineType::detect to
scan the entire byte slice and track two booleans (e.g., seen_crlf and seen_lf)
while iterating bytes, setting seen_crlf when a newline has a preceding '\r' and
seen_lf when a newline does not; after the loop return NewlineType::Unix if both
flags are set (mixed -> prefer LF), otherwise return Windows if only seen_crlf,
Unix if seen_lf, or Unix by default—adjust the logic inside detect accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c5ba2f3-013b-4cd4-86aa-32c56f7f3c0a
📒 Files selected for processing (5)
psl/psl/tests/reformat/reformat.rspsl/schema-ast/src/ast/newline_type.rspsl/schema-ast/src/lib.rspsl/schema-ast/src/reformat.rspsl/schema-ast/src/renderer.rs
Comment on lines
+1285
to
+1294
| fn mixed_input_defaults_to_lf() { | ||
| // First line uses LF, later lines use CRLF: the first newline wins | ||
| // and the reformatter falls back to LF for the whole output. | ||
| let input = format!( | ||
| "{}\n{}\r\n{}\r\n{}\r\n", | ||
| SCHEMA_LINES[0], SCHEMA_LINES[1], SCHEMA_LINES[2], SCHEMA_LINES[3] | ||
| ); | ||
| let out = reformat(&input); | ||
| assert!(!out.contains('\r'), "mixed input should normalize to LF: {out:?}"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
mixed_input_defaults_to_lf only validates one mixed ordering.
This case checks LF-first mixing only. Add a CRLF-first-then-LF variant to lock the mixed-ending contract from both directions.
Test addition
#[test]
fn mixed_input_defaults_to_lf() {
@@
let out = reformat(&input);
assert!(!out.contains('\r'), "mixed input should normalize to LF: {out:?}");
}
+
+#[test]
+fn mixed_input_crlf_first_also_defaults_to_lf() {
+ let input = format!(
+ "{}\r\n{}\n{}\r\n{}\r\n",
+ SCHEMA_LINES[0], SCHEMA_LINES[1], SCHEMA_LINES[2], SCHEMA_LINES[3]
+ );
+ let out = reformat(&input);
+ assert!(!out.contains('\r'), "mixed input should normalize to LF: {out:?}");
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@psl/psl/tests/reformat/reformat.rs` around lines 1285 - 1294, Add a second
test variant that mirrors mixed_input_defaults_to_lf but with CRLF on the first
line(s) and LF later to ensure normalization behavior is symmetric; duplicate
the test logic in a new function (e.g., mixed_input_with_crlf_first) using the
same SCHEMA_LINES sequence but build input so the first newline is "\r\n" and
later lines use "\n" (or construct format with CRLF-first then LF), call
reformat(&input) and assert no '\r' in out exactly like the original test to
lock the mixed-ending contract from both directions.
Comment on lines
+36
to
+47
| pub fn detect(input: &str) -> NewlineType { | ||
| let bytes = input.as_bytes(); | ||
| for (i, &b) in bytes.iter().enumerate() { | ||
| if b == b'\n' { | ||
| if i > 0 && bytes[i - 1] == b'\r' { | ||
| return NewlineType::Windows; | ||
| } | ||
| return NewlineType::Unix; | ||
| } | ||
| } | ||
| NewlineType::Unix | ||
| } |
There was a problem hiding this comment.
Mixed-ending detection does not actually guarantee LF fallback.
Line 36 returns on the first newline only, so CRLF-first mixed input is classified as Windows instead of LF. That contradicts the mixed-input contract in Lines 34-35 and can reintroduce inconsistent behavior.
Suggested fix
pub fn detect(input: &str) -> NewlineType {
let bytes = input.as_bytes();
+ let mut saw_lf = false;
+ let mut saw_crlf = false;
+
for (i, &b) in bytes.iter().enumerate() {
if b == b'\n' {
if i > 0 && bytes[i - 1] == b'\r' {
- return NewlineType::Windows;
+ saw_crlf = true;
+ } else {
+ saw_lf = true;
}
- return NewlineType::Unix;
+
+ if saw_lf && saw_crlf {
+ return NewlineType::Unix;
+ }
}
}
- NewlineType::Unix
+
+ if saw_crlf {
+ NewlineType::Windows
+ } else {
+ NewlineType::Unix
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn detect(input: &str) -> NewlineType { | |
| let bytes = input.as_bytes(); | |
| for (i, &b) in bytes.iter().enumerate() { | |
| if b == b'\n' { | |
| if i > 0 && bytes[i - 1] == b'\r' { | |
| return NewlineType::Windows; | |
| } | |
| return NewlineType::Unix; | |
| } | |
| } | |
| NewlineType::Unix | |
| } | |
| pub fn detect(input: &str) -> NewlineType { | |
| let bytes = input.as_bytes(); | |
| let mut saw_lf = false; | |
| let mut saw_crlf = false; | |
| for (i, &b) in bytes.iter().enumerate() { | |
| if b == b'\n' { | |
| if i > 0 && bytes[i - 1] == b'\r' { | |
| saw_crlf = true; | |
| } else { | |
| saw_lf = true; | |
| } | |
| if saw_lf && saw_crlf { | |
| return NewlineType::Unix; | |
| } | |
| } | |
| } | |
| if saw_crlf { | |
| NewlineType::Windows | |
| } else { | |
| NewlineType::Unix | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@psl/schema-ast/src/ast/newline_type.rs` around lines 36 - 47, The detect
function currently returns on the first newline and misclassifies mixed CRLF+LF
input as Windows; update NewlineType::detect to scan the entire byte slice and
track two booleans (e.g., seen_crlf and seen_lf) while iterating bytes, setting
seen_crlf when a newline has a preceding '\r' and seen_lf when a newline does
not; after the loop return NewlineType::Unix if both flags are set (mixed ->
prefer LF), otherwise return Windows if only seen_crlf, Unix if seen_lf, or Unix
by default—adjust the logic inside detect accordingly.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 prisma/prisma#8548 —
prisma formatemitted LF for every line of its output but added a CRLF as the trailing newline when the input used CRLF anywhere, leaving Windows users with mixed line endings (CRLF only on the last line).Per maintainer guidance from @SevInf (prisma/prisma#8548 comment), this fix lives in the Rust formatter (
psl/schema-ast/src/reformat.rs) rather than the TS CLI, and the line-ending decision is part of the renderer state — no post-process string replace.Changes
NewlineType::detect(&str) -> NewlineTypeinpsl/schema-ast/src/ast/newline_type.rs— first newline in the input wins; mixed-ending inputs fall back to LF per maintainer note.Renderernow carries aline_ending: NewlineTypefield;end_line()writes the configured separator natively (not LF + later replace).reformat()auto-detects the input line ending and delegates to a new siblingreformat_with_line_ending()that exposes explicit control. The trailing-newline guard appends the configured ending too.How tested
Added
psl/psl/tests/reformat/reformat.rs::line_endingscovering:reformat_with_line_endingLF / CRLF.Differs from prior PRs
prisma/prismaTS CLI (#29245, #29299, #29373, #29460, #29475, #29561, #29569, #29570 — closed for wrong target).Rendererstate, so each emitted line writes the configured separator rather than the formatter emitting LF and then string-replacing post-hoc (the rejection reason for fix(psl): preserve schema line endings during reformat #5812).Closes prisma/prisma#8548
/claim prisma/prisma#8548