Support csv writer QUOTE_STRINGS and QUOTE_NOTNULL#8109
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:
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFixes a panic in ChangesCSV QUOTE_STRINGS and QUOTE_NOTNULL writer implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
1172-1173: 💤 Low valueUnused lock acquisition.
The
_statelock guard is acquired but never used in this method since it builds output manually rather than using thecsv_core::WriterinWriteState. While harmless, this unnecessary synchronization could be removed.Proposed fix
fn writerow_quoted_strings(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { - let _state = self.state.lock(); let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| {🤖 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 1172 - 1173, In the writerow_quoted_strings method, remove the unnecessary lock acquisition line `let _state = self.state.lock();` since the lock guard is never actually used in the method. The method builds output manually without relying on the csv_core::Writer in WriteState, so this synchronization is not needed and can be safely deleted.
🤖 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 1138-1141: The code is unconditionally doubling the escape
character inside quoted fields when it encounters dialect.escapechar, but when
doublequote=True (the default), only quotes should be doubled, not escape
characters. Modify the logic that checks if dialect.escapechar == Some(byte) to
only apply the escape character doubling when doublequote=False. If doublequote
is True, skip the conditional block that pushes the byte an extra time for
escape characters, allowing only the single push outside the condition to
execute.
---
Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 1172-1173: In the writerow_quoted_strings method, remove the
unnecessary lock acquisition line `let _state = self.state.lock();` since the
lock guard is never actually used in the method. The method builds output
manually without relying on the csv_core::Writer in WriteState, so this
synchronization is not needed and can be safely deleted.
🪄 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: 4b5c06f1-7810-4acc-a372-e3408986c961
📒 Files selected for processing (2)
crates/stdlib/src/csv.rsextra_tests/snippets/stdlib_csv.py
I was surprised this wasn't already covered by a CPython test too :) Thanks for review! |
|
Probably adding a test to cpython will be also worth to do if you are interested in |
doma17
force-pushed
the
fix-csv-quote-strings-notnull
branch
from
e14d586 to
eb9f111
Compare
|
I updated the commit message and PR description to follow the AI policy template. |
Implement writer handling for QUOTE_STRINGS and QUOTE_NOTNULL so the exported constants no longer panic and follow CPython behavior for string, non-null, None, and quotechar validation cases. Constraint: RustPython contribution policy requires CPython-compatible behavior through Rust-side changes and regression coverage outside Lib/. Rejected: Marking CPython Lib tests or hiding the constants | would preserve a runtime panic for supported public constants. Confidence: high Scope-risk: narrow Directive: Keep QUOTE_STRINGS and QUOTE_NOTNULL writer behavior aligned with CPython's csv module when extending dialect validation. Tested: cargo check -p rustpython-stdlib; cargo run --release -- extra_tests/snippets/stdlib_csv.py; cargo run --release -- -m test test_csv; cargo clippy; pre-commit run --all-files via temporary venv/PATH shim; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher -- --test-threads=1 Not-tested: Remote CI and PR creation were not run because remote git operations require explicit approval. Assisted-by: Codex:gpt-5.5
Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
Apply maintainer-requested expression cleanups and rustfmt wrapping so the PR lint job can pass without changing behavior. Constraint: GitHub Actions Lint failed on rustfmt output after the review suggestion. Rejected: Changing csv writer semantics | the failing check was formatting-only and all behavior tests already passed. Confidence: high Scope-risk: narrow Directive: Keep future review suggestions rustfmt-clean before pushing. Tested: cargo fmt --check; cargo check -p rustpython-stdlib; cargo run --release -- extra_tests/snippets/stdlib_csv.py; cargo run --release -- -m test test_csv; cargo clippy; pre-commit run --all-files; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher -- --test-threads=1 Not-tested: Remote CI has not rerun yet; this commit will trigger it after push. Assisted-by: Codex:gpt-5.5
doma17
force-pushed
the
fix-csv-quote-strings-notnull
branch
from
eb9f111 to
031bd14
Compare
3 participants
Summary
Implements writer support for
csv.QUOTE_STRINGSandcsv.QUOTE_NOTNULL.These constants were already exposed, but using either quote style with
csv.writerreached an internaltodo!()and panicked.Problem
csv.writer(..., quoting=csv.QUOTE_STRINGS)andcsv.writer(..., quoting=csv.QUOTE_NOTNULL)should behave like CPython.Instead, RustPython converted these quote styles through an unimplemented path,
so normal Python code could terminate the interpreter with a Rust panic.
Fixes
QUOTE_STRINGSandQUOTE_NOTNULLaway from thetodo!()writer path.QUOTE_STRINGSquotes original string fields and still quotes fields thatrequire quoting because of delimiters, quote characters, or line endings.
QUOTE_NOTNULLquotes every field exceptNone.Noneis written as an empty unquoted field.None.quotechar=Nonewhen quoting is enabled.extra_tests/snippets/stdlib_csv.py.Testing
stdlib_csvregression with RustPython.test_csvpasses.AI use
Assisted by codex gpt-5.5 for implementation and test drafting. I reviewed, edited, and verified the changes locally.
Summary by CodeRabbit
Bug Fixes
QUOTE_STRINGSandQUOTE_NOTNULL, ensuring fields are correctly quoted and special characters are handled per the active dialect.quotecharis set toNone.New Features
Tests