-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Dedent -c code argument #8056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Dedent -c code argument #8056
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,7 +122,11 @@ fn parse_args() -> Result<(CliArgs, RunMode, Vec<String>), lexopt::Error> { | |
| Short('B') => args.dont_write_bytecode = true, | ||
| Short('c') => { | ||
| let cmd = parser.value()?.string()?; | ||
| return Ok((args, RunMode::Command(cmd), argv("-c".to_owned(), parser)?)); | ||
| return Ok(( | ||
| args, | ||
| RunMode::Command(dedent(&cmd)), | ||
| argv("-c".to_owned(), parser)?, | ||
| )); | ||
| } | ||
| Short('d') => args.debug += 1, | ||
| Short('E') => args.ignore_environment = true, | ||
|
|
@@ -465,3 +469,70 @@ pub(crate) fn split_paths<T: AsRef<std::ffi::OsStr> + ?Sized>( | |
| .into() | ||
| }) | ||
| } | ||
|
|
||
| /// Remove common whitespace prefix from all lines in a string. | ||
| /// | ||
| /// This is like textwrap.dedent, and is used to process -c's code | ||
| /// argument. It's different from ruff's dedent, which does not | ||
| /// distinguish between tab and space characters when dedenting. | ||
| fn dedent(input: &str) -> String { | ||
| let mut prefix: Option<String> = None; | ||
| let isspace = |c| c == ' ' || c == '\t'; | ||
|
|
||
| // All-whitespace lines become empty. | ||
| let deblanked: Vec<&str> = input | ||
| .lines() | ||
| .map(|line| if line.chars().all(isspace) { "" } else { line }) | ||
| .collect(); | ||
|
|
||
| // Find maximum common whitespace prefix, if any. | ||
| for line in deblanked.iter() { | ||
| if line.is_empty() { | ||
| continue; | ||
| } | ||
| if let Some(ref mut pstr) = prefix { | ||
| for (i, (c, pc)) in line.chars().zip(pstr.chars()).enumerate() { | ||
| // It's okay if `line` is shorter than `pstr`. At | ||
| // least one char in `line` must be non-whitespace, | ||
| // and if `line` is shorter than `pstr`, this | ||
| // non-whitespace char will be compared to a | ||
| // whitespace char, and loop will terminate. | ||
| if c != pc { | ||
| pstr.truncate(i); | ||
| break; | ||
| } | ||
| } | ||
|
Comment on lines
+494
to
+504
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical bug: Prefix not truncated when line is shorter than current prefix. The Example: if 🐛 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: 🤖 Prompt for AI Agents
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the example given,
In general, any This is possibly not very obvious; I will add a comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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
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. |
||
| } else { | ||
| let mut pstr = String::new(); | ||
| for c in line.chars() { | ||
| if isspace(c) { | ||
| pstr.push(c); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| prefix = Some(pstr); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if let Some(pstr) = prefix { | ||
| // Strip common prefix. | ||
| deblanked | ||
| .iter() | ||
| .map(|line| { | ||
| if line.is_empty() { | ||
| String::from("") | ||
| } else { | ||
| // All non-empty lines start with pstr, must be at | ||
| // least pstr.len() long. | ||
| String::from(line.get(pstr.len()..).unwrap()) | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("\n") | ||
| } else { | ||
| // No prefix found: all lines blank. | ||
| deblanked.join("\n") | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info!
I'm wondering if that's intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaReiser you know more about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed a bit in astral-sh/ruff#4731 (comment) when
dedentwas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply @MichaReiser :)
Will ruff be open for a PR patching
detentbehavior 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar enough with
textwrap.dedentand how it exactly differs from Ruff. I think a PR probably gives us most signal