csv: fix csv escape fieldsep#8260
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 dialect escape characters are now passed to reader builders. Quote-none dialects with an escape character use dedicated byte-level parsing for delimiters, spaces, newlines, field limits, trailing escapes, and UTF-8 conversion. ChangesCSV escapechar support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IterNext
participant read_quote_none_record
participant PythonRecord
IterNext->>read_quote_none_record: parse raw input bytes
read_quote_none_record->>PythonRecord: convert fields to Python strings
read_quote_none_record-->>IterNext: return record list or CSV error
IterNext-->>IterNext: increment line number
Possibly related PRs
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: 1
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
799-839: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated dialect-resolution logic across the three branches.
Each branch (
Str,Obj, default"excel") repeats the identical.delimiter(...).double_quote(...).escape(...)chain plus thequotecharcheck, differing only in whichPyDialectis used. Since this PR is adding a third repeated line (.escape(...)) to all three branches, it's a good point to extract the differing value once.♻️ Proposed refactor
fn to_reader(&self) -> csv_core::Reader { - let mut builder = csv_core::ReaderBuilder::new(); - let mut reader = match &self.dialect { - DialectItem::Str(name) => { - let g = GLOBAL_HASHMAP.lock(); - if let Some(dialect) = g.get(name) { - let mut builder = builder - .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote) - .escape(dialect.escapechar); - if let Some(t) = dialect.quotechar { - builder = builder.quote(t); - } - builder - } else { - &mut builder - } - } - DialectItem::Obj(obj) => { - let mut builder = builder - .delimiter(obj.delimiter) - .double_quote(obj.doublequote) - .escape(obj.escapechar); - if let Some(t) = obj.quotechar { - builder = builder.quote(t); - } - builder - } - _ => { - let name = "excel"; - let g = GLOBAL_HASHMAP.lock(); - let dialect = g.get(name).unwrap(); - let mut builder = builder - .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote) - .escape(dialect.escapechar); - if let Some(quotechar) = dialect.quotechar { - builder = builder.quote(quotechar); - } - builder - } - }; + let resolved = match &self.dialect { + DialectItem::Str(name) => GLOBAL_HASHMAP.lock().get(name).copied(), + DialectItem::Obj(obj) => Some(*obj), + _ => GLOBAL_HASHMAP.lock().get("excel").copied(), + }; + + let mut builder = csv_core::ReaderBuilder::new(); + let mut reader = if let Some(dialect) = resolved { + let mut b = builder + .delimiter(dialect.delimiter) + .double_quote(dialect.doublequote) + .escape(dialect.escapechar); + if let Some(t) = dialect.quotechar { + b = b.quote(t); + } + b + } else { + &mut builder + };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."
🤖 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 799 - 839, Refactor to_reader so the DialectItem::Str, DialectItem::Obj, and default "excel" branches first resolve a single PyDialect reference, then apply delimiter, double_quote, escape, and optional quotechar configuration once. Preserve the existing named-dialect lookup and fallback behavior while eliminating the duplicated ReaderBuilder configuration chains.Source: Coding guidelines
🤖 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 978-1032: Update read_quote_none_record to honor
dialect.skipinitialspace by removing leading spaces only when starting a field
immediately after a delimiter, while preserving spaces elsewhere and escaped
spaces. Add a regression test covering QUOTE_NONE with escapechar and
skipinitialspace for escaped-space input.
---
Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 799-839: Refactor to_reader so the DialectItem::Str,
DialectItem::Obj, and default "excel" branches first resolve a single PyDialect
reference, then apply delimiter, double_quote, escape, and optional quotechar
configuration once. Preserve the existing named-dialect lookup and fallback
behavior while eliminating the duplicated ReaderBuilder configuration chains.
🪄 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: 60b0182f-6a3a-4c1d-9c6e-ea201467be5f
⛔ Files ignored due to path filters (1)
Lib/test/test_csv.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/csv.rs
ReaderBuilder only received escapechar when it was passed directly as a keyword argument. A dialect object's or registered dialect's escapechar was visible through reader.dialect but not used by csv_core. Pass escapechar through for named, object, and default dialect paths. This makes escaped delimiters inside quoted fields follow the configured dialect, for example '"abc\,def"' reads as 'abc,def'. Unmark TestQuotedEscapedExcel.test_read_escape_fieldsep. Assisted-by: Codex
csv_core applies escapechar while reading quoted fields, but its unquoted field state treats an escaped delimiter as a delimiter. Consequently, QUOTE_NONE with escapechar split 'abc\,def' into 'abc\' and 'def'. Add a small QUOTE_NONE record parser for dialects with escapechar. It keeps the byte following an escape character as field data, so escaped delimiters do not end a field while ordinary delimiters and record terminators retain their usual meaning. Unmark TestEscapedExcel.test_read_escape_fieldsep. Assisted-by: Codex
The QUOTE_NONE parser returned before the normal reader's whitespace preprocessing, so it ignored skipinitialspace when escapechar was set. Track whether parsing has just followed a delimiter and skip only ordinary spaces in that position. Escaped spaces remain field data. Add a stdlib_csv snippet regression test for QUOTE_NONE with escapechar and skipinitialspace. Assisted-by: Codex:gpt-5.6-terra
to_reader configured ReaderBuilder separately for named, object, and default dialects even though the configuration was identical. Resolve the PyDialect once, then apply delimiter, double_quote, escapechar, and quotechar in one place. Preserve the existing unregistered-name fallback and required excel dialect lookup behavior. Assisted-by: Codex:gpt-5.6-terra
93ee871 to
0ef9af9
Compare
|
thank you for the approval and comment 😊! |
Summary
csv.readerdid not pass a dialect'sescapecharintocsv_core, so anescape character worked only when supplied as an explicit keyword argument.
The first commit passes the dialect value through to
csv_core, fixing escapeddelimiters inside quoted fields.
csv_coredoes not apply its escape setting to unquoted fields. The secondcommit adds a small RustPython
QUOTE_NONEparser path: an escape charactermakes the following byte data, so an escaped delimiter does not split a field.
Summary by CodeRabbit
escapechar, ensuring consistent behavior across dialect resolution paths.QUOTE_NONEhandling withescapecharandskipinitialspace, including correct escaped separators and whitespace processing.QUOTE_NONEwithescapecharandskipinitialspaceto verify correct field parsing.