csv: support multi-character lineterminator in writer - #8328
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCSV dialects now retain arbitrary non-empty ASCII line terminator strings. Readers use CRLF row recognition, while writers replace a csv-core sentinel with the configured terminator. Tests cover quoting modes, dialect registration, reader behavior, and invalid non-ASCII values. 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/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
c6565d0 to
5659610
Compare
|
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.
There was a problem hiding this comment.
Thanks @doma17, good catch.
I deferred non-ASCII terminators on purpose since doing it properly is a much bigger change — delimiter/quotechar/escapechar are still single bytes, so it only really makes sense together with full Unicode dialect support (#8310). But I only wrote that down in the issue and PR description and never enforced it in code, so the parser accepted non-ASCII and then handled it byte-wise. Didn't think to guard it there.
I'll reject non-ASCII terminators for now (csv.Error) and leave full support to the #8310 follow-up.
There was a problem hiding this comment.
@jinmay A golden rule when you know caveat is leaving a comment here is a caveat. please add a comment about future TODO about this.
There was a problem hiding this comment.
Sorry, I should have put that in the code in the first place rather than only in the PR description. Added a TODO at this line and on the validation helper, both pointing at #8310 for the code-point-wise handling.
youknowone
left a comment
There was a problem hiding this comment.
Looks good in general, please also fix build failure
Replace the csv-core reader and per-item quote scanner with one Rust implementation of CPython's nine-state reader parser. Keep parser state across iterator items and distinguish virtual item boundaries from true iterator exhaustion. Centralize field completion so quote provenance, empty-field None conversion, float conversion, strict parsing, field limits, blank rows, and escaped or quoted newlines share one path. Preserve the reentrant-iterator generation guard and the post-RustPython#8328 writer behavior. Remove the expected-failure markers from the eleven reader tests that now pass. Assisted-by: Codex:gpt-5.6-sol
f0f1e8c to
7c5a962
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extra_tests/snippets/stdlib_csv.py (1)
232-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWiden QUOTE_NONE escaping coverage to all terminator bytes.
This only verifies that
!(one byte of the three-byte terminator!@#) gets escaped. A field containing all three bytes together (e.g."a!@#b") would better validate that the per-byte escaping loop correctly handles a multi-character terminator end-to-end.♻️ Suggested broader case
none = io.StringIO() csv.writer( none, lineterminator="!@#", quoting=csv.QUOTE_NONE, escapechar="\\" ).writerow(["a!b", "x"]) assert none.getvalue() == "a\\!b,x!@#", none.getvalue() + + none_all = io.StringIO() + csv.writer( + none_all, lineterminator="!@#", quoting=csv.QUOTE_NONE, escapechar="\\" + ).writerow(["a!@#b", "x"]) + assert none_all.getvalue() == "a\\!\\@\\`#b`,x!@#", none_all.getvalue()🤖 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 `@extra_tests/snippets/stdlib_csv.py` around lines 232 - 237, Expand the QUOTE_NONE test around the csv.writer call to use a field containing all terminator bytes together, such as “a!@#b”, and update the expected escaped output accordingly so the assertion verifies each byte in the multi-character lineterminator is escaped.
🤖 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.
Nitpick comments:
In `@extra_tests/snippets/stdlib_csv.py`:
- Around line 232-237: Expand the QUOTE_NONE test around the csv.writer call to
use a field containing all terminator bytes together, such as “a!@#b”, and
update the expected escaped output accordingly so the assertion verifies each
byte in the multi-character lineterminator is escaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b770ed4d-605d-4c30-915e-7466e37f6a5c
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/csv.rs
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.
The writer decides what to quote and escape by comparing raw bytes, so a non-ASCII terminator quoted a field that merely shared a UTF-8 lead byte, and QUOTE_NONE escaped individual bytes of the terminator and then failed to decode the record back to a string. Reject non-ASCII terminators, including lone surrogates, as csv.Error when the dialect is parsed, and leave code-point-wise handling to a follow-up (RustPython#8310). The non-string error message now matches CPython as well.
7c5a962 to
61e9782
Compare
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_NONEwithescapechar.\r\nwhile keeping other terminator text inside fields (CPython-aligned).lineterminatorvalues and reject non-ASCII inputs.lineterminatorbehavior across quoting modes and reader/writer round-tripping.lineterminatorvalues.