Normalize blank lines in bottom mode - #17
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBlank-line normalization is centralized for import blocks and complete output. Standard and bottom processing remove excessive and trailing blank lines, preserve CRLF endings, track normalization changes, and select full-file diffs when needed. Tests cover these behaviors and idempotence. ChangesBlank-line normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/zsort_test.zig (1)
421-434: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect bottom-mode
changedexpectations.Bottom mode sets
changedby comparing the source with normalized output.
src/zsort_test.zig#L421-L434: expectresult.changedand compare against output without the final blank line.src/zsort_test.zig#L624-L653: expect!result.changedbecause the source already equals the expected normalized output.🤖 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 `@src/zsort_test.zig` around lines 421 - 434, Update the bottom-mode test at src/zsort_test.zig lines 421-434 to expect result.changed and compare result.new_text with the normalized output that omits the final blank line. Also update the test at src/zsort_test.zig lines 624-653 to expect !result.changed because its source already matches the expected normalized output.src/zsort.zig (1)
786-821: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet
changedfrom the normalized complete output.
collapseBlankLinescan modifyrestafter Line 806 calculateschanged. For example, a sorted import block followed by two blank lines before a body declaration returns normalizednew_textbut reportschanged = false. Fix mode then skips the rewrite.Compare
sourcewithcollapsedafter normalization. If complete-output normalization changed text outside the import block, make check mode use the full-text diff path at Line 1221. Add a standard-layout regression test for this case.🤖 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 `@src/zsort.zig` around lines 786 - 821, Update the changed calculation in the block-processing flow to compare the original source with the normalized collapsed output, so changes introduced by collapseBlankLines are detected. Ensure check mode uses the full-text diff path when normalization changes text outside the import block, and add a standard-layout regression test covering excessive blank lines between sorted imports and a body declaration.
🤖 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.
Outside diff comments:
In `@src/zsort_test.zig`:
- Around line 421-434: Update the bottom-mode test at src/zsort_test.zig lines
421-434 to expect result.changed and compare result.new_text with the normalized
output that omits the final blank line. Also update the test at
src/zsort_test.zig lines 624-653 to expect !result.changed because its source
already matches the expected normalized output.
In `@src/zsort.zig`:
- Around line 786-821: Update the changed calculation in the block-processing
flow to compare the original source with the normalized collapsed output, so
changes introduced by collapseBlankLines are detected. Ensure check mode uses
the full-text diff path when normalization changes text outside the import
block, and add a standard-layout regression test covering excessive blank lines
between sorted imports and a body declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 827af227-2a25-4de1-8cac-8f0b0232a3b2
📒 Files selected for processing (2)
src/zsort.zigsrc/zsort_test.zig
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zsort.zig (1)
789-807: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDetect whitespace-only separator lines before inserting
junction_blank.If
reststarts with a blank line such as" \r\n",rest[0]is a space. This condition inserts another newline.collapseBlankLinesthen keeps the inserted line and removes the original first blank line. The output can lose the original blank line’s whitespace or line-ending representation.Inspect the first line of
restwith the same trim rule used bycollapseBlankLines.Suggested fix
- const junction_blank = !has_trailing_comment and !endsWithBlankLine(new_imports) and rest.len > 0 and rest[0] != '\n' and rest[0] != '\r'; + const rest_starts_blank = rest.len > 0 and + std.mem.trim(u8, rest[0..findLineEnd(rest, 0)], " \t\r\n").len == 0; + const junction_blank = !has_trailing_comment and !endsWithBlankLine(new_imports) and + rest.len > 0 and !rest_starts_blank;🤖 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 `@src/zsort.zig` around lines 789 - 807, Update the junction_blank calculation near last_span_end so it inspects the first line of rest using the same std.mem.trim whitespace rule as collapseBlankLines, rather than checking rest[0] for newline characters. Treat whitespace-only first lines as existing separators and preserve their original whitespace and line-ending representation.
🤖 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.
Outside diff comments:
In `@src/zsort.zig`:
- Around line 789-807: Update the junction_blank calculation near last_span_end
so it inspects the first line of rest using the same std.mem.trim whitespace
rule as collapseBlankLines, rather than checking rest[0] for newline characters.
Treat whitespace-only first lines as existing separators and preserve their
original whitespace and line-ending representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d89f6a66-1716-4a2e-abbc-50860aad61eb
📒 Files selected for processing (2)
src/zsort.zigsrc/zsort_test.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/zsort_test.zig
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zsort.zig (1)
798-807: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the blank-line predicate at the import/body seam.
When
reststarts with a whitespace-only line such as" \r\n",junction_blankadds another separator.collapseBlankLineskeeps the inserted separator and discards the original line, which changes its whitespace or line-ending representation. Detectrest_starts_blankwithstd.mem.trimand apply the same predicate inendsWithBlankLine. Add a regression test with spaces or tabs in the separator.🤖 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 `@src/zsort.zig` around lines 798 - 807, Update the import/body seam logic around junction_blank and endsWithBlankLine to detect rest_starts_blank using std.mem.trim, then reuse that predicate when deciding whether to add the separator. Preserve the original whitespace and line-ending representation when rest begins with a whitespace-only line, and add a regression test covering spaces or tabs in that separator.
🤖 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.
Outside diff comments:
In `@src/zsort.zig`:
- Around line 798-807: Update the import/body seam logic around junction_blank
and endsWithBlankLine to detect rest_starts_blank using std.mem.trim, then reuse
that predicate when deciding whether to add the separator. Preserve the original
whitespace and line-ending representation when rest begins with a
whitespace-only line, and add a regression test covering spaces or tabs in that
separator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d96c0206-7757-4ee4-b0ad-9bc32ae15ecf
📒 Files selected for processing (2)
src/zsort.zigsrc/zsort_test.zig
1 participant
Summary by CodeRabbit
Bug Fixes
Tests