Skip to content

csv: quote embedded CR/LF under QUOTE_MINIMAL #8315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Jul 19, 2026
+95 −2
Merged

csv: quote embedded CR/LF under QUOTE_MINIMAL #8315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b32aa3
csv: quote embedded CR/LF under QUOTE_MINIMAL with custom lineterminator
jinmay Jul 18, 2026
071c799
csv: unmark now-passing writer tests
jinmay Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Lib/test/test_csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ def test_write_lineterminator(self):
f'1,2{lineterminator}'
f'"\r","\n"{lineterminator}')

@unittest.expectedFailure # TODO: RUSTPYTHON
def test_write_iterable(self):
self._write_test(iter(['a', 1, 'p,q']), 'a,1,"p,q"')
self._write_test(iter(['a', 1, None]), 'a,1,')
Expand Down Expand Up @@ -319,7 +318,6 @@ def test_writerows_with_none(self):
self.assertEqual(fileobj.read(), 'a\r\n""\r\n')


@unittest.expectedFailure # TODO: RUSTPYTHON
def test_write_empty_fields(self):
self._write_test((), '')
self._write_test([''], '""')
Expand Down
50 changes: 50 additions & 0 deletions crates/stdlib/src/csv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,13 +1380,63 @@ mod _csv {
self.write.call((s,), vm)
}

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)
}
Comment on lines +1383 to +1430

@coderabbitai coderabbitai Bot Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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


#[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),
Comment on lines 1433 to +1439

@coderabbitai coderabbitai Bot Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.

_ => {}
}

Expand Down
45 changes: 45 additions & 0 deletions extra_tests/snippets/stdlib_csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,48 @@ def test_quote_none_reader_skipinitialspace_escapechar():


test_quote_none_reader_skipinitialspace_escapechar()


def test_quote_minimal_writer_lineterminator():
# https://github.com/RustPython/RustPython/issues/8302
# QUOTE_MINIMAL must quote '\r' and '\n' regardless of the line terminator.
buf = io.StringIO()
writer = csv.writer(buf, lineterminator="!")
writer.writerow(["a", "b"])
writer.writerow([1, 2])
writer.writerow(["\r", "\n"])
assert buf.getvalue() == 'a,b!1,2!"\r","\n"!'

nul = io.StringIO()
csv.writer(nul, lineterminator="\0").writerow(["\r", "\n"])
assert nul.getvalue() == '"\r","\n"\0'

crlf = io.StringIO()
csv.writer(crlf, lineterminator="!").writerow(["\r\n"])
assert crlf.getvalue() == '"\r\n"!'

# the terminator character itself still triggers quoting
term = io.StringIO()
csv.writer(term, lineterminator="!").writerow(["a!b", "c"])
assert term.getvalue() == '"a!b",c!'

# default terminator behavior is unchanged
default = io.StringIO()
csv.writer(default).writerow(["\r", "\n"])
assert default.getvalue() == '"\r","\n"\r\n'


test_quote_minimal_writer_lineterminator()


def test_quote_minimal_writer_empty_fields():
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow([""])
writer.writerow([None])
writer.writerow([])
writer.writerow(["", ""])
assert buf.getvalue() == '""\r\n""\r\n\r\n,\r\n'


test_quote_minimal_writer_empty_fields()
Loading