csv: handle empty fields with skipinitialspace#8304
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)
📝 WalkthroughWalkthroughThe CSV reader replaces its unquoted parser with a shared quote-aware parser, preserves quote provenance during field conversion, updates parser routing and initial-space trimming, and adds a regression test for spaces inside quoted fields. ChangesCSV quote-aware parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Reader
participant RecordParser
participant FieldConversion
Reader->>RecordParser: select quote-aware parsing mode
RecordParser->>RecordParser: scan quotes, escapes, delimiters, and terminators
RecordParser->>FieldConversion: provide decoded fields with quote provenance
FieldConversion-->>Reader: return converted CSV row
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1164-1170: Update the skipinitialspace preprocessing in the CSV
parsing path so it does not split or trim delimiter-separated segments inside
quoted fields. Make the trimming logic quote- and escape-aware, or move it into
the parser, preserving spaces within quoted values while still removing initial
spaces from unquoted fields.
- Around line 1086-1100: Update the field conversion closure in the CSV parsing
path to handle QuoteStyle::Strings distinctly: preserve quoted fields as
strings, return None for empty unquoted fields, and parse nonempty unquoted
fields as floats. Use the existing VM float-construction/parsing utilities and
retain current UTF-8 error handling for values that remain strings.
🪄 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: 6daadf40-d52b-4fa5-b1b1-353e391a14de
⛔ 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
| assert list(reader) == [["a", "b, c", "d"]] | ||
|
|
||
|
|
||
| test_reader_skipinitialspace_preserves_quoted_spaces() |
There was a problem hiding this comment.
isnt this test covered by test_read_skipinitialspace in test_csv? in that case, we don't need this test. could you check if it is that case or not?
There was a problem hiding this comment.
test_read_skipinitialspace only covers unquoted input. Its QUOTE_NOTNULL and QUOTE_STRINGS cases contain blank fields, while the other cases use the default QUOTE_MINIMAL mode.
It does not cover the following case, where a quoted field contains a delimiter followed by a space:
reader = csv.reader(['a, "b, c", d'], skipinitialspace=True)The implementation in 51a96d5 trimmed the input without respecting quote boundaries, causing the CPython incompatibility described in this review comment. so I added this regression case to extra_tests.
skipinitialspace preprocessed records by trimming both ends of every split field. Fields containing only spaces could therefore produce an invalid slice, while trailing spaces were removed even though CPython preserves them. Trim only field prefixes in the csv-core path and make the remaining all-space trimming helper return an empty slice safely. QUOTE_NOTNULL and QUOTE_STRINGS add another distinction: an unquoted empty field becomes None, but a quoted empty field remains an empty string. Both forms have identical decoded bytes, so extend the existing QUOTE_NONE custom reader into one shared path that records whether each field started quoted. Use that metadata only for the null conversion while retaining QUOTE_NONE escape behavior. Unmark Test_Csv.test_read_skipinitialspace now that its standard, QUOTE_NOTNULL, and QUOTE_STRINGS cases pass. This keeps the existing one-item reader lifecycle and does not add the larger strict or multiline parser state machine. Assisted-by: Codex:gpt-5-sol
The csv-core preprocessing path split each raw iterator item on every delimiter before trimming field prefixes. Delimiters inside quoted fields were therefore treated as separators, so spaces after them could be removed before csv-core parsed the record. Extract the quote, escape, delimiter, and field-start transitions from read_quote_record() into a small per-item scanner. Reuse its events in the skipinitialspace preprocessor and discard only InitialSpace events while copying all other source bytes unchanged. This keeps the custom quote-mode reader and preprocessing logic aligned without introducing persistent multiline or strict parser state. Remove the delimiter cache that was only needed by the old split-and-join path and add a regression for spaces following a delimiter inside a quoted field. Assisted-by: Codex:gpt-5-sol
352ecf7 to
0d784ab
Compare
3 participants
Summary
csv.reader(..., skipinitialspace=True)preprocessed each record by splittingit on the delimiter and trimming both ends of every piece. This had two
problems:
For example, the old preprocessing could change:
'a, "b, c", d'before passing it to csv-core, removing the space inside "b, c".
The raw split-and-join preprocessing is now removed. A small per-item quote scanner classifies field starts, quote boundaries, doubled quotes, escapes, delimiters, and record terminators. The skipinitialspace preprocessing discards only actual initial-space events and copies every other source byte unchanged.
The same scanner is consumed by the custom quote-mode reader, avoiding separate implementations of the quote and escape transitions.
QUOTE_NOTNULL and QUOTE_STRINGS
The CPython test also covers
QUOTE_NOTNULLandQUOTE_STRINGS. For thesemodes, an empty unquoted field must become
None, while a quoted empty fieldmust remain an empty string:
Both fields have empty decoded byte contents, so the conversion cannot be
decided from the final field buffer alone. The reader must retain whether each
field began with a quote.
The existing custom QUOTE_NONE reader is therefore extended into a shared
reader path for:
Each parsed field carries its bytes and a was_quoted flag. The flag is used
only when QUOTE_NOTNULL or QUOTE_STRINGS converts an empty unquoted field
to None. This avoids separate, duplicated functions for the two modes while
preserving the existing QUOTE_NONE escape behavior.
Test_Csv.test_read_skipinitialspace is unmarked now that its regular,
QUOTE_NOTNULL, and QUOTE_STRINGS cases pass.
Scope
This is intentionally a narrow reader fix. It does not introduce a complete
CPython-style CSV state machine and does not change:
Testing
temp/workspace/before-commit-csv.sh passed, including:
cargo fmt --check
cargo run --release Lib/test/test_csv.py
cargo run -- extra_tests/snippets/stdlib_csv.py
cargo clippy -p rustpython-stdlib --all-targets
the configured workspace test suite
related issue
Summary by CodeRabbit
Summary by CodeRabbit
skipinitialspace=True, initial-space trimming is now quote-aware, preserving spaces inside quoted fields.escapecharis configured.skipinitialspace=Trueensuring spaces inside quoted fields are preserved.