Dedent -c code argument - #8056
Merged
Merged
Conversation
Contributor
|
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 (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/typing.py dependencies:
dependent tests: (19 tests)
[ ] lib: cpython/Lib/pdb.py dependencies:
dependent tests: (1 tests)
[ ] lib: cpython/Lib/traceback.py dependencies:
dependent tests: (159 tests)
[x] lib: cpython/Lib/decimal.py dependencies:
dependent tests: (73 tests)
[ ] lib: cpython/Lib/glob.py dependencies:
dependent tests: (56 tests)
[ ] lib: cpython/Lib/enum.py dependencies:
dependent tests: (16 tests)
[x] lib: cpython/Lib/io.py dependencies:
dependent tests: (108 tests)
[ ] lib: cpython/Lib/imaplib.py dependencies:
dependent tests: (1 tests)
[ ] lib: cpython/Lib/ntpath.py dependencies:
dependent tests: (51 tests)
[x] test: cpython/Lib/test/test_select.py (TODO: 3) dependencies: dependent tests: (109 tests)
[x] lib: cpython/Lib/pydoc.py dependencies:
dependent tests: (5 tests)
[x] lib: cpython/Lib/tempfile.py dependencies:
dependent tests: (91 tests)
[ ] lib: cpython/Lib/encodings dependencies:
dependent tests: (158 tests)
[ ] test: cpython/Lib/test/test_pwd.py dependencies: dependent tests: (159 tests)
[x] test: cpython/Lib/test/test_thread.py (TODO: 3) dependencies: dependent tests: (13 tests) [ ] lib: cpython/Lib/base64.py dependencies:
dependent tests: (55 tests)
[ ] lib: cpython/Lib/gzip.py dependencies:
dependent tests: (35 tests)
[ ] lib: cpython/Lib/asyncio dependencies:
dependent tests: (6 tests)
[ ] lib: cpython/Lib/ftplib.py dependencies:
dependent tests: (36 tests)
[ ] lib: cpython/Lib/collections dependencies:
dependent tests: (326 tests)
[ ] lib: cpython/Lib/http dependencies:
dependent tests: (49 tests)
[x] lib: cpython/Lib/tty.py dependencies:
dependent tests: (6 tests)
[ ] lib: cpython/Lib/email dependencies:
dependent tests: (52 tests)
[ ] lib: cpython/Lib/tarfile.py dependencies:
dependent tests: (98 tests)
[ ] test: cpython/Lib/test/test_pyrepl (TODO: 41) dependencies: dependent tests: (no tests depend on pyrepl) [ ] test: cpython/Lib/test/test_utf8_mode.py (TODO: 6) dependencies: dependent tests: (no tests depend on utf8_mode) [ ] lib: cpython/Lib/json dependencies:
dependent tests: (12 tests)
[ ] lib: cpython/Lib/locale.py dependencies:
dependent tests: (103 tests)
[ ] test: cpython/Lib/test/test_bytes.py (TODO: 22) dependencies: dependent tests: (no tests depend on bytes) [x] test: cpython/Lib/test/test_termios.py (TODO: 15) dependencies: dependent tests: (8 tests)
[ ] lib: cpython/Lib/rlcompleter.py dependencies:
dependent tests: (3 tests)
[ ] test: cpython/Lib/test/test_cmd_line.py (TODO: 24) dependencies: dependent tests: (no tests depend on cmd_line) [ ] lib: cpython/Lib/compression dependencies:
dependent tests: (99 tests)
[ ] test: cpython/Lib/test/test_grp.py dependencies: dependent tests: (152 tests)
[x] lib: cpython/Lib/mimetypes.py dependencies:
dependent tests: (41 tests)
[ ] lib: cpython/Lib/os.py dependencies:
dependent tests: (185 tests)
[x] lib: cpython/Lib/hashlib.py dependencies:
dependent tests: (142 tests)
[ ] lib: cpython/Lib/shutil.py dependencies:
dependent tests: (249 tests)
[ ] test: cpython/Lib/test/test_unpack.py (TODO: 1) dependencies: dependent tests: (no tests depend on unpack) [x] lib: cpython/Lib/bz2.py dependencies:
dependent tests: (100 tests)
[ ] lib: cpython/Lib/_pyrepl dependencies:
dependent tests: (7 tests)
[ ] lib: cpython/Lib/functools.py dependencies:
dependent tests: (49 tests)
[x] lib: cpython/Lib/pty.py dependencies:
dependent tests: (4 tests)
[x] test: cpython/Lib/test/test_list.py (TODO: 4) dependencies: dependent tests: (no tests depend on list) [ ] test: cpython/Lib/test/test_import (TODO: 3) dependencies: dependent tests: (no tests depend on import) [x] lib: cpython/Lib/code.py dependencies:
dependent tests: (2 tests) Legend:
|
Contributor
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
Comment on lines
+486
to
+491
| for (i, (c, pc)) in line.chars().zip(pstr.chars()).enumerate() { | ||
| if c != pc { | ||
| pstr.truncate(i); | ||
| break; | ||
| } | ||
| } |
Contributor
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.
Contributor
Author
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.
Contributor
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
approved these changes
Jun 7, 2026
ShaharNaveh
left a comment
Contributor
There was a problem hiding this comment.
TYSM!
Welcome to the project:)
ShaharNaveh
reviewed
Jun 7, 2026
| } | ||
|
|
||
| ///like textwrap.dedent; for processing -c argument | ||
| fn dedent(input: &str) -> String { |
Contributor
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"
Contributor
Author
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?
Contributor
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
Contributor
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.
Contributor
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
Contributor
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
Contributor
Author
Thank you! I forgot to run |
Contributor
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
requested changes
Jun 10, 2026
youknowone
left a comment
Member
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?
roryyorke
force-pushed
the
rory/test_cmd_line
branch
from
June 14, 2026 07:31
31d9128 to
60f10b2
Compare
Contributor
Author
That's embarrassing --- thank you for letting me know. I've amended the commits and force pushed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 by automatically stripping common leading indentation from multi-line commands, ensuring consistently normalized execution when commands are formatted with extra whitespace.