csv: quote embedded CR/LF under QUOTE_MINIMAL#8315
Conversation
QUOTE_MINIMAL rows were serialized by csv-core, which only quotes bytes registered for the configured terminator, so fields containing '\r' or '\n' were left unquoted when the lineterminator was not CR/LF (e.g. '!' or '\0'). Route QUOTE_MINIMAL through a hand-written path that uses field_needs_quotes, which always treats '\r'/'\n' as needing quotes. A single empty field is quoted and an empty row emits only the terminator, matching CPython. QUOTE_ALL / QUOTE_NONNUMERIC still use csv-core. Assisted-by: Claude Code:claude-opus-4-8
test_write_iterable and test_write_empty_fields pass with the QUOTE_MINIMAL writer fix (empty rows no longer emit an empty field). Assisted-by: Claude Code:claude-opus-4-8
📝 WalkthroughWalkthroughThe CSV writer now implements ChangesCSV minimal quoting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extra_tests/snippets/stdlib_csv.py (1)
155-155: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSpecify
newline=""inio.StringIOto prevent platform-dependent newline translation.
io.StringIO()defaults to translating\ntoos.linesepwhen writing. On Windows, this translates the\r\nwritten bycsv.writerinto\r\r\n, which will cause your strictgetvalue()assertions to fail. The Python CSV module documentation specifically recommends passingnewline=""to avoid this gotcha. The shared root cause applies to all test instantiations:
extra_tests/snippets/stdlib_csv.py#L155-L155: addnewline=""toio.StringIOinstantiation.extra_tests/snippets/stdlib_csv.py#L162-L162: addnewline=""toio.StringIOinstantiation.extra_tests/snippets/stdlib_csv.py#L166-L166: addnewline=""toio.StringIOinstantiation.extra_tests/snippets/stdlib_csv.py#L171-L171: addnewline=""toio.StringIOinstantiation.extra_tests/snippets/stdlib_csv.py#L176-L176: addnewline=""toio.StringIOinstantiation.extra_tests/snippets/stdlib_csv.py#L185-L185: addnewline=""toio.StringIOinstantiation.🤖 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` at line 155, Update every io.StringIO instantiation in extra_tests/snippets/stdlib_csv.py at lines 155-155, 162-162, 166-166, 171-171, 176-176, and 185-185 to pass newline="".
🤖 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 1383-1430: Consolidate the duplicated handwritten quoting logic by
replacing writerow_minimal, writerow_quote_none, and writerow_quoted_strings
with one writerow_custom method. Centralize row iteration, field conversion,
quoting-style selection for Strings, Notnull, Minimal, and None,
single-empty-field validation, output construction, and line-terminator writing
in writerow_custom, then update callers to use it and remove the obsolete
helpers.
- Around line 1433-1439: Update the writerow method’s quoting-style dispatch so
all handwritten quoting styles route through the unified writerow_custom helper
introduced by the refactor, while preserving the existing behavior for other
styles and the QuoteStyle matching structure.
---
Nitpick comments:
In `@extra_tests/snippets/stdlib_csv.py`:
- Line 155: Update every io.StringIO instantiation in
extra_tests/snippets/stdlib_csv.py at lines 155-155, 162-162, 166-166, 171-171,
176-176, and 185-185 to pass newline="".
🪄 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: 8ac39457-ccab-4cd2-ae56-f3b77bf26e4b
⛔ 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
| fn writerow_minimal(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { | ||
| let _state = self.state.lock(); | ||
|
|
||
| let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| { | ||
| new_csv_error( | ||
| vm, | ||
| format!("'{}' object is not iterable", row.class().name()), | ||
| ) | ||
| })?; | ||
|
|
||
| let fields = row.iter(vm)?.collect::<PyResult<Vec<_>>>()?; | ||
| let single_field = fields.len() == 1; | ||
| let mut output = Vec::new(); | ||
|
|
||
| for (index, field) in fields.into_iter().enumerate() { | ||
| if index > 0 { | ||
| output.push(self.dialect.delimiter); | ||
| } | ||
|
|
||
| let stringified; | ||
| let data: &[u8] = match_class!(match field { | ||
| ref s @ PyStr => s.as_bytes(), | ||
| crate::builtins::PyNone => b"", | ||
| ref obj => { | ||
| stringified = obj.str(vm)?; | ||
| stringified.as_bytes() | ||
| } | ||
| }); | ||
|
|
||
| // CPython quotes a QUOTE_MINIMAL field if it contains the | ||
| // delimiter, the quote character, '\r', '\n', or the line | ||
| // terminator, regardless of which line terminator is | ||
| // configured. A row with a single empty field is also quoted | ||
| // so that it is not read back as an empty line. | ||
| if field_needs_quotes(data, self.dialect) || (single_field && data.is_empty()) { | ||
| write_quoted_field(&mut output, data, self.dialect, vm)?; | ||
| } else { | ||
| output.extend_from_slice(data); | ||
| } | ||
| } | ||
|
|
||
| write_lineterminator(&mut output, self.dialect.lineterminator); | ||
|
|
||
| let s = core::str::from_utf8(&output) | ||
| .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?; | ||
|
|
||
| self.write.call((s,), vm) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Consolidate duplicated writerow helpers into a single method.
As per coding guidelines, when branches share common logic, the logic should be extracted to avoid duplication. The new writerow_minimal is largely a copy of writerow_quote_none and writerow_quoted_strings (duplicating row iteration, string conversion, and line-terminator logic).
Consider extracting a single writerow_custom method that handles all handwritten quoting styles and replacing all three redundant helpers.
♻️ Proposed unified implementation
Add this unified method, delete writerow_minimal, and safely remove the now-dead writerow_quote_none and writerow_quoted_strings methods:
fn writerow_custom(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
let _state = self.state.lock();
let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| {
new_csv_error(
vm,
format!("'{}' object is not iterable", row.class().name()),
)
})?;
let fields = row.iter(vm)?.collect::<PyResult<Vec<_>>>()?;
let single_field = fields.len() == 1;
let mut output = Vec::new();
for (index, field) in fields.into_iter().enumerate() {
if index > 0 {
output.push(self.dialect.delimiter);
}
let stringified;
let (data, is_str, is_none): (&[u8], bool, bool) = match_class!(match field {
ref s @ PyStr => (s.as_bytes(), true, false),
crate::builtins::PyNone => (b"", false, true),
ref obj => {
stringified = obj.str(vm)?;
(stringified.as_bytes(), false, false)
}
});
let (should_quote, error_if_single_empty, use_unquoted) = match self.dialect.quoting {
QuoteStyle::Strings => (is_str || field_needs_quotes(data, self.dialect), true, false),
QuoteStyle::Notnull => (!is_none, true, false),
QuoteStyle::Minimal => (field_needs_quotes(data, self.dialect) || (single_field && data.is_empty()), false, false),
QuoteStyle::None => (false, true, true),
_ => unreachable!(),
};
if should_quote {
write_quoted_field(&mut output, data, self.dialect, vm)?;
} else {
if error_if_single_empty && single_field && data.is_empty() {
return Err(new_csv_error(
vm,
"single empty field record must be quoted",
));
}
if use_unquoted {
write_unquoted_field(&mut output, data, self.dialect, vm)?;
} else {
output.extend_from_slice(data);
}
}
}
write_lineterminator(&mut output, self.dialect.lineterminator);
let s = core::str::from_utf8(&output)
.map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?;
self.write.call((s,), vm)
}🤖 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 1383 - 1430, Consolidate the
duplicated handwritten quoting logic by replacing writerow_minimal,
writerow_quote_none, and writerow_quoted_strings with one writerow_custom
method. Centralize row iteration, field conversion, quoting-style selection for
Strings, Notnull, Minimal, and None, single-empty-field validation, output
construction, and line-terminator writing in writerow_custom, then update
callers to use it and remove the obsolete helpers.
Source: Coding guidelines
| fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { | ||
| match self.dialect.quoting { | ||
| QuoteStyle::None => return self.writerow_quote_none(row, vm), | ||
| QuoteStyle::Strings | QuoteStyle::Notnull => { | ||
| return self.writerow_quoted_strings(row, vm); | ||
| } | ||
| QuoteStyle::Minimal => return self.writerow_minimal(row, vm), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update writerow dispatch to use the unified helper.
If you adopt the writerow_custom refactor, update this match arm to dispatch all handwritten styles to the unified method.
♻️ Proposed fix
- #[pymethod]
- fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
- match self.dialect.quoting {
- QuoteStyle::None => return self.writerow_quote_none(row, vm),
- QuoteStyle::Strings | QuoteStyle::Notnull => {
- return self.writerow_quoted_strings(row, vm);
- }
- QuoteStyle::Minimal => return self.writerow_minimal(row, vm),
- _ => {}
- }
+ #[pymethod]
+ fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
+ if matches!(
+ self.dialect.quoting,
+ QuoteStyle::None | QuoteStyle::Strings | QuoteStyle::Notnull | QuoteStyle::Minimal
+ ) {
+ return self.writerow_custom(row, vm);
+ }📝 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.
| fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { | |
| match self.dialect.quoting { | |
| QuoteStyle::None => return self.writerow_quote_none(row, vm), | |
| QuoteStyle::Strings | QuoteStyle::Notnull => { | |
| return self.writerow_quoted_strings(row, vm); | |
| } | |
| QuoteStyle::Minimal => return self.writerow_minimal(row, vm), | |
| fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { | |
| if matches!( | |
| self.dialect.quoting, | |
| QuoteStyle::None | QuoteStyle::Strings | QuoteStyle::Notnull | QuoteStyle::Minimal | |
| ) { | |
| return self.writerow_custom(row, vm); | |
| } |
🤖 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 1433 - 1439, Update the writerow
method’s quoting-style dispatch so all handwritten quoting styles route through
the unified writerow_custom helper introduced by the refactor, while preserving
the existing behavior for other styles and the QuoteStyle matching structure.
|
Re: CodeRabbit's suggestion to consolidate the hand-written writer paths ( The duplication is real. I'd like to keep this PR scoped to the QUOTE_MINIMAL fix, though: |
Summary
Under
QUOTE_MINIMAL, a field containing\ror\nmust be quoted regardless of thelineterminator— CPython quotes them unconditionally. RustPython serializedQUOTE_MINIMALrows withcsv_core, which decides quoting from the bytes it registered for the terminator, so with a terminator other than CR/LF (e.g.'!'or'\0') embedded\r/\nwere left unquoted.QUOTE_MINIMALnow uses a hand-written path (likeQUOTE_NONE/QUOTE_STRINGS) that decides quoting via the existingfield_needs_quotes, which always treats\r/\nas needing quotes.QUOTE_ALLandQUOTE_NONNUMERICstill usecsv_core.This also fixes an empty row (
writerow([])) emitting""instead of only the terminator.Multi-character
lineterminator(the!@#case intest_write_lineterminator) is still unsupported and left for a follow-up, so that test stays marked.Test plan
test_write_iterableandtest_write_empty_fields(both@unittest.expectedFailurebefore).cargo run --release -- -m test test_csv: 128 run, 7 skipped, SUCCESS.extra_tests/snippets/stdlib_csv.py: passes (added writer cases for custom terminators and empty fields).cargo fmt --check/cargo clippy: clean (no new warnings).QUOTE_MINIMALwriter outputs match.Assisted-by: Claude Code:claude-opus-4-8
Summary by CodeRabbit
Bug Fixes
Tests