Dedent -c code argument#8056
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 selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesCommand-line -c dedent preprocessing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/os.py dependencies:
dependent tests: (183 tests)
[ ] test: cpython/Lib/test/test_cmd_line.py (TODO: 24) dependencies: dependent tests: (no tests depend on cmd_line) 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: 1
🧹 Nitpick comments (2)
src/settings.rs (2)
474-474: 💤 Low valueInline comments should have a space after
//.The Rust style guide recommends a space between
//and the comment text for readability.✨ Suggested improvements
- //all-whitespace lines become empty + // all-whitespace lines become empty let deblanked: Vec<&str> = input- //find maximum common whitespace prefix, if any + // find maximum common whitespace prefix, if any for line in deblanked.iter() {- //strip prefix + // strip prefix deblanked- //no prefix: all lines blank + // no prefix: all lines blank deblanked.join("\n")Also applies to: 480-480, 507-507, 522-522
🤖 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 `@src/settings.rs` at line 474, Fix the inline comment spacing by ensuring there is a single space after the comment slashes (change occurrences like "//all-whitespace lines become empty" to "// all-whitespace lines become empty" and likewise for the other inline comments in the same section), updating each inline comment that currently lacks a space after "//" so they follow Rust style guide spacing.
469-469: 💤 Low valueDoc comment style: Capitalize and punctuate properly.
Rust doc comments should start with a capital letter and end with a period.
✨ Suggested improvement
-///like textwrap.dedent; for processing -c argument +/// Remove common leading whitespace from lines, like textwrap.dedent. fn dedent(input: &str) -> String {🤖 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 `@src/settings.rs` at line 469, Update the doc comment currently written as "///like textwrap.dedent; for processing -c argument" to follow Rust doc style: start with a capital letter and end with a period; e.g. change it to something like "/// Like textwrap::dedent. Used for processing the -c argument." Locate the exact doc comment string to modify so the surrounding item (the function/const handling inline `-c` processing) gets the corrected comment.
🤖 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 `@src/settings.rs`:
- Around line 486-491: The loop using line.chars().zip(pstr.chars()) can leave
pstr too long when line is shorter; change it to iterate with char_indices (e.g.
line.char_indices().zip(pstr.char_indices()), tracking a matched_byte_offset
variable that you update to pi + pc.len_utf8() on each matched pair and set to
the correct byte offset when a mismatch occurs, and after the loop always call
pstr.truncate(matched_byte_offset) so pstr is correctly truncated to the true
common prefix; reference the existing variables/operations pstr, line, and
pstr.truncate in your fix.
---
Nitpick comments:
In `@src/settings.rs`:
- Line 474: Fix the inline comment spacing by ensuring there is a single space
after the comment slashes (change occurrences like "//all-whitespace lines
become empty" to "// all-whitespace lines become empty" and likewise for the
other inline comments in the same section), updating each inline comment that
currently lacks a space after "//" so they follow Rust style guide spacing.
- Line 469: Update the doc comment currently written as "///like
textwrap.dedent; for processing -c argument" to follow Rust doc style: start
with a capital letter and end with a period; e.g. change it to something like
"/// Like textwrap::dedent. Used for processing the -c argument." Locate the
exact doc comment string to modify so the surrounding item (the function/const
handling inline `-c` processing) gets the corrected comment.
🪄 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: 9e488883-5a71-49b7-a6d1-00f7ff8f724f
⛔ Files ignored due to path filters (2)
Lib/test/test_cmd_line.pyis excluded by!Lib/**Lib/test/test_os.pyis excluded by!Lib/**
📒 Files selected for processing (1)
src/settings.rs
| for (i, (c, pc)) in line.chars().zip(pstr.chars()).enumerate() { | ||
| if c != pc { | ||
| pstr.truncate(i); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: Prefix not truncated when line is shorter than current prefix.
The zip terminates when either iterator ends. If line has fewer characters than pstr, the loop completes without calling truncate, leaving pstr longer than the actual common prefix.
Example: if pstr = " " (4 spaces) and line = " x" (2 spaces + x), zip yields only 2 character pairs, both matching. The loop never calls truncate, so pstr incorrectly remains 4 spaces instead of being reduced to 2.
🐛 Proposed fix to track matched length and always truncate
if let Some(ref mut pstr) = prefix {
+ let mut matched = 0;
for (i, (c, pc)) in line.chars().zip(pstr.chars()).enumerate() {
if c != pc {
pstr.truncate(i);
break;
}
+ matched = i + 1;
}
- } else {
+ // Truncate to matched length (handles case where line is shorter than pstr)
+ let byte_idx = pstr.char_indices()
+ .nth(matched)
+ .map(|(idx, _)| idx)
+ .unwrap_or(pstr.len());
+ pstr.truncate(byte_idx);
+ } else {Note: truncate takes a byte index, so we must map the character count to the byte offset via char_indices.
🤖 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 `@src/settings.rs` around lines 486 - 491, The loop using
line.chars().zip(pstr.chars()) can leave pstr too long when line is shorter;
change it to iterate with char_indices (e.g.
line.char_indices().zip(pstr.char_indices()), tracking a matched_byte_offset
variable that you update to pi + pc.len_utf8() on each matched pair and set to
the correct byte offset when a mismatch occurs, and after the loop always call
pstr.truncate(matched_byte_offset) so pstr is correctly truncated to the true
common prefix; reference the existing variables/operations pstr, line, and
pstr.truncate in your fix.
There was a problem hiding this comment.
in the example given, pstr = " " and line = " x", zip will yield 3 character pairs; the tuples (i, (c, pc))will be
(0, (' ', ' '))- match, continue(1, (' ', ' '))- match, continue(2, (' ', 'x'))- mismatch, truncate prefix to length 2
In general, any line processed here must have at least one non-whitespace character; since pstr comprises only whitespace, either pstr matches line up to the length of pstr (no truncation), or some element of pstr does not match, and pstr will be truncated. In particular, if line is shorter than pstr, a non-whitespace character from line must be compared to a whitespace character from pstr before either zipped iterator is exhausted.
This is possibly not very obvious; I will add a comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List my comments on PR `#8056` to find the comment ID to resolve
gh api repos/RustPython/RustPython/pulls/8056/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}'Length of output: 374
🏁 Script executed:
#!/bin/bash
# Resolve the review comment by marking it as resolved
gh api repos/RustPython/RustPython/pulls/8056/reviews --jq '.'
# Use the GraphQL API to resolve the review thread
gh api graphql -f query='
query {
repository(owner: "RustPython", name: "RustPython") {
pullRequest(number: 8056) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
author { login }
body
}
}
}
}
}
}
}'Length of output: 18198
🏁 Script executed:
#!/bin/bash
# Resolve the review thread using GraphQL
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOCA8Bec6HpK50"}) {
thread {
id
isResolved
}
}
}'Length of output: 719
@roryyorke You're correct, and I apologize for the false positive. My mental model of the zip was wrong — since line.chars() is the left iterator and all non-empty lines are guaranteed to contain at least one non-whitespace character (while pstr is purely whitespace), a mismatch will always be found before either iterator is exhausted when line is shorter than pstr. The logic is sound.
Adding a comment to document that invariant is a great idea for future readers. I attempted to resolve this review thread programmatically but don't have the necessary permissions — you're welcome to resolve it manually.
🐰 scratch scratch ...oh, the bug was a ghost.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
TYSM!
Welcome to the project:)
| } | ||
|
|
||
| ///like textwrap.dedent; for processing -c argument | ||
| fn dedent(input: &str) -> String { |
There was a problem hiding this comment.
Would: https://github.com/astral-sh/ruff/blob/5ac2a53ee108f23dc40a63d5378a0d6b21d821c3/crates/ruff_python_trivia/src/textwrap.rs#L134-L185 work as well? (you can use that method directly, no need to copy it here)
There was a problem hiding this comment.
Ruff's dedent does not match CPython's behavior.
I checked its source. Ruff's dedent only counts the leading space. But CPython's impl and textwrap.dedent also checks its content.
For example,
cpython.dedent("\t 1\n \t1") "\t 1\n \t1"
ruff.dedent("\t 1\n \t1") "1\n1"There was a problem hiding this comment.
I wasn't aware of ruff's dedent. It's interesting that it doesn't distinguish tab and space -- I wonder if that's deliberate?
There was a problem hiding this comment.
Ruff's
dedentdoes not match CPython's behavior.I checked its source. Ruff's
dedentonly counts the leading space. But CPython's impl andtextwrap.dedentalso checks its content.For example,
cpython.dedent("\t 1\n \t1") "\t 1\n \t1" ruff.dedent("\t 1\n \t1") "1\n1"
Thanks for the info!
I'm wondering if that's intentional
There was a problem hiding this comment.
This was discussed a bit in astral-sh/ruff#4731 (comment) when dedent was introduced to Ruff. There's also a test (https://github.com/astral-sh/ruff/blob/04008db76b3876d3576366e839c5e8067368d117/crates/ruff_python_trivia/src/textwrap.rs#L423-L435). I suspect that the reasoning simply was that this is fine for Ruff where we only apply fixes if the source code is valid. But it might also just have been a simplification to ship faster.
There was a problem hiding this comment.
I suspect that the reasoning simply was that this is fine for Ruff where we only apply fixes if the source code is valid. But it might also just have been a simplification to ship faster.
Thanks for the reply @MichaReiser :)
Will ruff be open for a PR patching detent behavior to behave like CPython's textwrap.dedent?
Although, I'm not sure what impact will this have on ruff. I can open a PR and see if there's any failing tests because of it if you'd be fine with merging it
There was a problem hiding this comment.
I'm not familiar enough with textwrap.dedent and how it exactly differs from Ruff. I think a PR probably gives us most signal
Thank you! I forgot to run |
dw about the CI, put whenever you feel like it:) Can you please add a commend saying that this is different from ruff's impl? |
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Thank you for contributing!
You seem to use wrong email to your git commits. That might be your correct email address, but in that case it means you didn't add the email to your github account.
Could you please check your setup and fix to commits to have your correct email?
6 participants
I haven't used AI at all for this. (I'm not sure if an "anti-disclaimer" is needed...)
Summary
IndentationError, but now fails with aTypeError.Summary by CodeRabbit
-ccommand-line argument handling to strip common leading indentation from multi-line command strings, so indented or formatted commands are normalized before execution.