csv: support multi-character lineterminator in writer#8328
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCSV dialects now retain arbitrary non-empty line terminator strings. Readers use CRLF row recognition, while writers replace a csv-core sentinel with the configured terminator. Tests cover multi-character terminators across quoting modes, dialect registration, and reader behavior. ChangesCSV line terminator support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CSVAPI
participant FormatOptions
participant csv_core
participant Writer
CSVAPI->>FormatOptions: parse multi-character lineterminator
FormatOptions->>csv_core: configure sentinel writer terminator
Writer->>csv_core: serialize row
csv_core->>Writer: return row ending with sentinel
Writer->>CSVAPI: replace sentinel with configured terminator
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/csv.rs (1)
885-935: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRedundant sentinel-terminator setup duplicated across both dialect branches.
.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL))is set inside both theDialectItem::Str(Line 894) andDialectItem::Obj(Line 911) branches, and then unconditionally overwritten again with the identical value at Line 934. The two branch-local calls are dead code since Line 934 always re-applies the same sentinel regardless of dialect source — this is exactly the value-differs/logic-shared duplication the repo guideline calls out.As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
♻️ Proposed fix removing the redundant branch-local terminator calls
if let Some(dialect) = g.get(name) { let mut builder = builder .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote) - .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); + .double_quote(dialect.doublequote);DialectItem::Obj(obj) => { let mut builder = builder .delimiter(obj.delimiter) - .double_quote(obj.doublequote) - .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); + .double_quote(obj.doublequote);🤖 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 `@crates/stdlib/src/csv.rs` around lines 885 - 935, Remove the redundant .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)) calls from both DialectItem::Str and DialectItem::Obj builder branches in to_writer. Keep the existing unconditional terminator assignment after the dialect-specific and override handling so the sentinel is applied once for every dialect source.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
1488-1496: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCorrectness-critical sentinel invariant relies only on
debug_assert_eq!, which is compiled out in release builds.If the assumption that
buffer[buffer_offset - 1] == CSV_CORE_TERMINATOR_SENTINELever breaks (e.g. future change toto_writer/csv-core usage), release builds would silently drop the wrong byte and splice the real terminator into corrupted output instead of failing loudly. Given this is currently guaranteed by how csv-core'sterminator()is used, the risk is low today, but the guard should hold in release builds too.🛡️ Proposed fix to keep the check in release builds
- debug_assert_eq!(buffer[buffer_offset - 1], CSV_CORE_TERMINATOR_SENTINEL); + assert_eq!(buffer[buffer_offset - 1], CSV_CORE_TERMINATOR_SENTINEL);🤖 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 `@crates/stdlib/src/csv.rs` around lines 1488 - 1496, Replace the debug-only assertion in the output construction path with a release-enforced check that validates buffer[buffer_offset - 1] is CSV_CORE_TERMINATOR_SENTINEL before removing it; fail loudly if the invariant is violated, while preserving the existing terminator replacement behavior for valid output.
🤖 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 `@crates/stdlib/src/csv.rs`:
- Around line 623-632: The empty-lineterminator validation is duplicated with
inconsistent exception types and messages. Update FormatOptions::from_args at
crates/stdlib/src/csv.rs:623-632 and prase_lineterminator_from_obj at
crates/stdlib/src/csv.rs:223-236 to share one validation path, using the same
exception type and wording that describe the actual non-empty constraint;
preserve acceptance of non-empty strings, including multi-character values.
---
Outside diff comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 885-935: Remove the redundant
.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)) calls from both
DialectItem::Str and DialectItem::Obj builder branches in to_writer. Keep the
existing unconditional terminator assignment after the dialect-specific and
override handling so the sentinel is applied once for every dialect source.
---
Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 1488-1496: Replace the debug-only assertion in the output
construction path with a release-enforced check that validates
buffer[buffer_offset - 1] is CSV_CORE_TERMINATOR_SENTINEL before removing it;
fail loudly if the invariant is violated, while preserving the existing
terminator replacement behavior for valid output.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 79e824cb-2025-42fe-a920-917fb6699cf3
⛔ Files ignored due to path filters (1)
Lib/test/test_csv.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/src/csv.rsextra_tests/snippets/stdlib_csv.py
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/bz2.py dependencies:
dependent tests: (101 tests)
[x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
c6565d0 to
5659610
Compare
Store the dialect line terminator as an owned String instead of the single-byte csv_core::Terminator, dropping PyDialect's Copy derive. The manual writer paths emit the full terminator; the csv-core-backed QUOTE_ALL/QUOTE_NONNUMERIC paths emit a sentinel byte (preserving csv-core's quote/empty-record bookkeeping) and append the real terminator. field_needs_quotes/escape now quote a field containing any terminator byte. The reader ignores lineterminator and always uses CRLF, matching CPython and avoiding mid-UTF-8 record splits.
- Remove the redundant per-branch sentinel terminator setup in to_writer; the unconditional terminator call after the match is the single source. - Reword the empty-lineterminator errors to "must not be empty" (the constraint is non-empty, not single-character) on both entry points. - Promote the sentinel invariant check in writerow from debug_assert_eq! to assert_eq! so it also guards release builds. - Add a snippet case for a field containing a line-break byte to ensure the csv-core path drops only the trailing terminator.
|
Since #8304 was merged, the read_quote_none_record function has been removed, so this branch now has a merge conflict. That wasn't intentional—could you take a look? |
9e3fad7 to
f0f1e8c
Compare
|
@widehyo1 sure. I will! thank you for catching this |
| fn field_needs_quotes(data: &[u8], dialect: &PyDialect) -> bool { | ||
| data.iter().any(|&byte| { | ||
| byte == dialect.delimiter | ||
| || dialect.quotechar == Some(byte) | ||
| || matches!(byte, b'\r' | b'\n') | ||
| || matches!(dialect.lineterminator, Terminator::Any(t) if byte == t) | ||
| // CPython quotes a field that contains any character of the line | ||
| // terminator. This byte-wise `contains` matches CPython for | ||
| // ASCII terminators; non-ASCII terminators are not fully handled. | ||
| || dialect.lineterminator.as_bytes().contains(&byte) | ||
| }) | ||
| } | ||
|
|
||
| fn field_needs_escape(byte: u8, dialect: PyDialect) -> bool { | ||
| fn field_needs_escape(byte: u8, dialect: &PyDialect) -> bool { | ||
| byte == dialect.delimiter | ||
| || dialect.quotechar == Some(byte) | ||
| || dialect.escapechar == Some(byte) | ||
| || matches!(byte, b'\r' | b'\n') | ||
| || matches!(dialect.lineterminator, Terminator::Any(t) if byte == t) | ||
| || dialect.lineterminator.as_bytes().contains(&byte) | ||
| } |
There was a problem hiding this comment.
Hi @jinmay
Lineterminator accepts non-ASCII strings, but this byte-wise check can split UTF-8 fields.
Please reject them or handle them character-wise.
Summary
csv.writerrejected anylineterminatorthat was not a single character — including the default'\r\n'when passed explicitly — because the dialect stored it as a single-bytecsv_core::Terminator. CPython accepts an arbitrary-length terminator.The dialect now stores the line terminator as an owned
String(droppingPyDialect'sCopyderive). The hand-written writer paths (QUOTE_MINIMAL/QUOTE_NONE/QUOTE_STRINGS/QUOTE_NOTNULL) emit the full terminator directly. Thecsv_core-backedQUOTE_ALL/QUOTE_NONNUMERICpaths still rely oncsv_corefor its quoting bookkeeping, so they emit a one-byte sentinel terminator (keepingcsv_core's closing-quote / empty-record handling intact) and then replace that sentinel with the real terminator.field_needs_quotes/field_needs_escapenow quote/escape a field containing any byte of the terminator, matching CPython.The reader now ignores
lineterminatorand always splits on\r/\n/\r\n, matching CPython (csv.readernever honored the dialect terminator) and avoiding a mid-UTF-8 record split that a multi-byte terminator would otherwise cause.Scoped to ASCII terminators; non-ASCII/surrogate terminators and empty-terminator acceptance are left for a follow-up.
Test plan
test_write_lineterminator(@unittest.expectedFailurebefore).cargo run --release -- -m test test_csv: 128 run, 7 skipped, SUCCESS.extra_tests/snippets/stdlib_csv.py: passes (addedtest_multichar_lineterminatorcovering multi-char terminators, terminator-byte quoting,QUOTE_ALL/QUOTE_NONNUMERIC,QUOTE_NONEescaping,register_dialectround-trip, and reader behavior; verified on both RustPython and CPython).cargo fmt --check/cargo clippy: clean (no new warnings).Assisted-by: Claude Code:claude-opus-4-8
Summary by CodeRabbit
lineterminatorhandling to support arbitrary non-empty multi-character terminators.QUOTE_NONE(withescapechar).\r\n, leaving other terminator text intact as field content (CPython-aligned).lineterminatorvalue.lineterminatorbehavior across multiple quoting modes, dialect round-tripping, and reader behavior.