#59 Add beautification for print statements and string literals - #60
Conversation
…nvert triple-quoted strings in decompiled output
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new beautification pass in Changes
Sequence Diagram(s)(omitted — changes are a localized single-component post-processing pass and tests) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_integration.py (1)
376-380: Prefer semantic assertions over exact punctuation here.Like truth itself, this test should hold to essence, not ornament. These
assertInchecks are brittle on quote/escape/parenthesis style and may fail on valid decompiler output.Proposed refactor
- self.assertIn("print(\"This is my Input :\\n %s\" % in_a)", out_core) + self.assertRegex( + out_core, + r'print\((["\'])This is my Input :\\n %s\1 % in_a\)' + ) @@ - self.assertIn("print(f\"This is my Input2 : {in_a}\\' and {in_a}\")", out_core) + self.assertRegex( + out_core, + r'print\(f["\']This is my Input2 : \{in_a\}\\?\' and \{in_a\}["\']\)' + ) @@ - self.assertIn("print('This is my Input2 : \\'%s\\' and \"%s\"' % (in_a, in_a))", out_core) + self.assertRegex( + out_core, + r'print\((["\'])This is my Input2 : \\\'%s\\\' and \\"%s\\"\1 % \(in_a, in_a\)\)' + ) @@ - self.assertIn("print((\"\"\"This is my Input :\n %s\"\"\" % in_a))", out_none) + self.assertRegex( + out_none, + r'print\(\(*"""This is my Input :\n %s""" % in_a\)*\)' + )Also applies to: 385-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_integration.py` around lines 376 - 380, Replace the brittle exact-string assertions that depend on quoting/escaping with semantic checks against out_core: instead of asserting the whole print line text, assert that out_core contains the key substrings "This is my Input" and the variable name in_a for the first case, and for the second case assert that out_core contains "This is my Input2" and two occurrences of in_a (or use a regex like re.search(r'This is my Input2.*in_a.*in_a', out_core)) so the test passes regardless of quote/escape/formatting differences; update the two assertIn blocks that reference print(...) and the alternate f-string/percent-format branches accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pycrefine.py`:
- Around line 471-477: The current regex-based rewrite corrupts triple-quoted
strings containing quotes and also rewrites print(...) that appear inside other
string literals; replace this fragile approach by using the AST to locate actual
string literals and print calls: traverse the AST to find constant string nodes
(instead of raw regex on inner) and for each triple-quoted string node use
ast.literal_eval to obtain the true Python string value and then reserialize it
safely (choose quoting to avoid embedded quote corruption) rather than doing
m.group(1).replace(...), and ensure any transformation of print calls only
happens on AST Call nodes (not inside string constants) — alternatively, if you
must keep regex, add a robust helper like _line_is_in_triple_quoted_string() to
skip regex substitutions when inside a real string and use ast.literal_eval in
repl_str to parse and reserialize the literal safely.
- Around line 479-492: The current unwrapping logic (regex m on variable inner
and loop that sets inside → inner) can turn a single tuple argument into
multiple print arguments; modify the guard so you only unwrap when the
parentheses are purely grouping. Specifically, after matching m and computing
inside, refuse to unwrap if there is a top-level comma in inside (i.e., a comma
not nested in deeper parens/brackets/braces) or, more robustly, parse inside
with Python's ast (ast.parse(inner, mode='eval')) and only unwrap if the parsed
expression is not an ast.Tuple node (and not a starred or comma-separated
argument structure). Keep the existing balance check, then add the
top-level-comma/AST-Tuple check before assigning inner = inside.
---
Nitpick comments:
In `@tests/test_integration.py`:
- Around line 376-380: Replace the brittle exact-string assertions that depend
on quoting/escaping with semantic checks against out_core: instead of asserting
the whole print line text, assert that out_core contains the key substrings
"This is my Input" and the variable name in_a for the first case, and for the
second case assert that out_core contains "This is my Input2" and two
occurrences of in_a (or use a regex like re.search(r'This is my
Input2.*in_a.*in_a', out_core)) so the test passes regardless of
quote/escape/formatting differences; update the two assertIn blocks that
reference print(...) and the alternate f-string/percent-format branches
accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec676344-7c26-49e9-aa92-ef77d06c85db
📒 Files selected for processing (4)
pycrefine.pytest_files/verify_scenes.pytests/test_helpers.pytests/test_integration.py
| # convert triple quoted strings to literal newline strings | ||
| def repl_str(m): | ||
| string_content = m.group(1).replace('\n', '\\n') | ||
| return f'"{string_content}"' | ||
|
|
||
| inner = re.sub(r"'''([\s\S]*?)'''", repl_str, inner) | ||
| inner = re.sub(r'"""([\s\S]*?)"""', repl_str, inner) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
import re
def transform(text: str) -> str:
def fix_print_parens_and_strings(match):
prefix = match.group(1)
inner = match.group(2)
def repl_str(m):
string_content = m.group(1).replace('\n', '\\n')
return f'"{string_content}"'
inner = re.sub(r"'''([\s\S]*?)'''", repl_str, inner)
inner = re.sub(r'"""([\s\S]*?)"""', repl_str, inner)
m = re.match(r'^\((.*)\)$', inner, flags=re.DOTALL)
if m:
inside = m.group(1)
depth = 0
balanced = True
for char in inside:
if char == '(':
depth += 1
elif char == ')':
depth -= 1
if depth < 0:
balanced = False
break
if balanced and depth == 0:
inner = inside
return f"{prefix}{inner})"
return re.sub(
r'^([ \t]*print\s*\()([\s\S]*?)\)$',
fix_print_parens_and_strings,
text,
flags=re.MULTILINE,
)
src1 = 'print("""He said "peace".\nAnd moved on.""")\n'
out1 = transform(src1)
print("rewritten literal:", out1.rstrip())
try:
ast.parse(out1)
print("parse=ok")
except SyntaxError as e:
print("parse failed:", e)
src2 = 'doc = """\nprint((1, 2))\n"""\n'
out2 = transform(src2)
before = ast.parse(src2).body[0].value.value
after = ast.parse(out2).body[0].value.value
print("string before:", repr(before))
print("string after :", repr(after))
PYRepository: sahebbiswas/pycrefine
Length of output: 270
Address two grave defects in the regex-based string and print rewriting.
This transformation treats source code as plain text, yielding two critical failures:
First, repl_str() corrupts valid triple-quoted strings that contain " characters. The string """He said "peace".""" becomes "He said "peace".", which cannot parse.
Second, the regex operates upon the entire file without discernment, rewriting print(...) calls that dwell within the sanctuary of another string. Thus "\nprint((1, 2))\n" becomes corrupted to "\nprint(1, 2)\n", destroying user content.
The remedy is not difficult: employ syntax-aware parsing with ast, or at least guard transformations with _line_is_in_triple_quoted_string() and use ast.literal_eval() for safe literal rewriting. Truth demands respect for structure; this path alone is sustainable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` around lines 471 - 477, The current regex-based rewrite
corrupts triple-quoted strings containing quotes and also rewrites print(...)
that appear inside other string literals; replace this fragile approach by using
the AST to locate actual string literals and print calls: traverse the AST to
find constant string nodes (instead of raw regex on inner) and for each
triple-quoted string node use ast.literal_eval to obtain the true Python string
value and then reserialize it safely (choose quoting to avoid embedded quote
corruption) rather than doing m.group(1).replace(...), and ensure any
transformation of print calls only happens on AST Call nodes (not inside string
constants) — alternatively, if you must keep regex, add a robust helper like
_line_is_in_triple_quoted_string() to skip regex substitutions when inside a
real string and use ast.literal_eval in repl_str to parse and reserialize the
literal safely.
…is stripping in decompiler, and add integration test suite
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pycrefine.py (1)
520-521:⚠️ Potential issue | 🔴 CriticalPlease stop regex rewrites from touching string-literal content.
With respect, Line 521 still performs a global text rewrite, so
print(...)text inside triple-quoted literals/docstrings can still be mutated as code. This prior critical defect remains.Proposed safe guard (line-aware, skips triple-quoted literal regions)
- if beautification_level in ('core', 'aggressive'): - text = re.sub(r'^([ \t]*print\s*\()([\s\S]*?)\)$', fix_print_parens_and_strings, text, flags=re.MULTILINE) + if beautification_level in ('core', 'aggressive'): + _pp_lines = text.split('\n') + for _idx, _line in enumerate(_pp_lines): + if _line_is_in_triple_quoted_string(_pp_lines, _idx): + continue + _pp_lines[_idx] = re.sub( + r'^([ \t]*print\s*\()(.*)\)$', + fix_print_parens_and_strings, + _line, + ) + text = '\n'.join(_pp_lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 520 - 521, The global re.sub on text when beautification_level is 'core'|'aggressive' is touching triple-quoted string/docstring content; restrict the replacement to non-string regions by first parsing the file into spans and skipping STRING tokens (or triple-quoted regions) before applying fix_print_parens_and_strings. Concretely: locate the re.sub call that uses fix_print_parens_and_strings and replace it with logic that uses the Python tokenizer or a line-aware scanner to produce a list of (code|string) spans, run the regex only on spans flagged as code, then reassemble the text so print(...) inside triple-quoted string literals/docstrings remains unchanged.
🧹 Nitpick comments (1)
pycrefine.py (1)
476-482: Remove broad exception handling inrepl_str.Line 481 catches
Exceptionbroadly, but this block should not need it; this can hide real defects and triggers BLE001.Proposed simplification
- def repl_str(m): - raw_content = m.group(1) # exactly what was between the triple quotes - try: - # repr() of a str always produces a syntactically valid Python literal. - # We then strip the surrounding quotes repr adds so we can re-wrap below. - safe_repr = repr(raw_content) # e.g. "'hello\\nworld'" or '"it\'s"' - return safe_repr - except Exception: - # Fallback: escape newlines and wrap in double-quotes, escaping any - # embedded double-quotes to avoid broken output. - escaped = raw_content.replace('\\', '\\\\').replace('\n', '\\n').replace('"', '\\"') - return f'"{escaped}"' + def repl_str(m): + raw_content = m.group(1) + return repr(raw_content)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 476 - 482, In repl_str, remove the broad try/except around repr(raw_content) — repr on a str should not need to be guarded; delete the except Exception block and let errors propagate (or, if you want a narrow guard, catch a specific exception like TypeError). Locate the repr(raw_content) call and return safe_repr directly (ensure raw_content is actually a str before calling repr if necessary), and remove the fallback except branch so BLE001 is not triggered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pycrefine.py`:
- Around line 499-511: Several single-line if/elif statements in the
tuple-unwrapping logic should be expanded into multi-line blocks to satisfy Ruff
E701: replace "if char == '(': depth += 1" with a multi-line if that increments
depth, replace "elif char == ')': depth -= 1" with a multi-line elif that
decrements depth, replace "if depth < 0: balanced = False; break" with a
multi-line if that sets balanced = False and then breaks on separate lines, and
similarly replace "if char in '([{': d += 1" and "elif char in ')]}': d -= 1"
with multi-line if/elif blocks; keep the same variable names (depth, balanced,
inside, d) and the control flow otherwise unchanged.
---
Duplicate comments:
In `@pycrefine.py`:
- Around line 520-521: The global re.sub on text when beautification_level is
'core'|'aggressive' is touching triple-quoted string/docstring content; restrict
the replacement to non-string regions by first parsing the file into spans and
skipping STRING tokens (or triple-quoted regions) before applying
fix_print_parens_and_strings. Concretely: locate the re.sub call that uses
fix_print_parens_and_strings and replace it with logic that uses the Python
tokenizer or a line-aware scanner to produce a list of (code|string) spans, run
the regex only on spans flagged as code, then reassemble the text so print(...)
inside triple-quoted string literals/docstrings remains unchanged.
---
Nitpick comments:
In `@pycrefine.py`:
- Around line 476-482: In repl_str, remove the broad try/except around
repr(raw_content) — repr on a str should not need to be guarded; delete the
except Exception block and let errors propagate (or, if you want a narrow guard,
catch a specific exception like TypeError). Locate the repr(raw_content) call
and return safe_repr directly (ensure raw_content is actually a str before
calling repr if necessary), and remove the fallback except branch so BLE001 is
not triggered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed74876c-4535-4ff4-badf-1e5591b419c0
📒 Files selected for processing (2)
pycrefine.pytests/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_integration.py
| if char == '(': depth += 1 | ||
| elif char == ')': depth -= 1 | ||
| if depth < 0: | ||
| balanced = False | ||
| break | ||
| if balanced and depth == 0: | ||
| # Comma check: refuse to unwrap if there is a top-level comma | ||
| # (that would turn a single tuple argument into multiple args). | ||
| top_level_comma = False | ||
| d = 0 | ||
| for char in inside: | ||
| if char in '([{': d += 1 | ||
| elif char in ')]}': d -= 1 |
There was a problem hiding this comment.
Please split single-line if statements to satisfy Ruff E701.
Lines 499, 500, 510, and 511 use multiple statements on one line and currently fail lint.
Proposed lint-safe rewrite
for char in inside:
- if char == '(': depth += 1
- elif char == ')': depth -= 1
+ if char == '(':
+ depth += 1
+ elif char == ')':
+ depth -= 1
if depth < 0:
balanced = False
break
@@
d = 0
for char in inside:
- if char in '([{': d += 1
- elif char in ')]}': d -= 1
+ if char in '([{':
+ d += 1
+ elif char in ')]}':
+ d -= 1
elif char == ',' and d == 0:
top_level_comma = True
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if char == '(': depth += 1 | |
| elif char == ')': depth -= 1 | |
| if depth < 0: | |
| balanced = False | |
| break | |
| if balanced and depth == 0: | |
| # Comma check: refuse to unwrap if there is a top-level comma | |
| # (that would turn a single tuple argument into multiple args). | |
| top_level_comma = False | |
| d = 0 | |
| for char in inside: | |
| if char in '([{': d += 1 | |
| elif char in ')]}': d -= 1 | |
| if char == '(': | |
| depth += 1 | |
| elif char == ')': | |
| depth -= 1 | |
| if depth < 0: | |
| balanced = False | |
| break | |
| if balanced and depth == 0: | |
| # Comma check: refuse to unwrap if there is a top-level comma | |
| # (that would turn a single tuple argument into multiple args). | |
| top_level_comma = False | |
| d = 0 | |
| for char in inside: | |
| if char in '([{': | |
| d += 1 | |
| elif char in ')]}': | |
| d -= 1 |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 499-499: Multiple statements on one line (colon)
(E701)
[error] 500-500: Multiple statements on one line (colon)
(E701)
[error] 510-510: Multiple statements on one line (colon)
(E701)
[error] 511-511: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` around lines 499 - 511, Several single-line if/elif statements
in the tuple-unwrapping logic should be expanded into multi-line blocks to
satisfy Ruff E701: replace "if char == '(': depth += 1" with a multi-line if
that increments depth, replace "elif char == ')': depth -= 1" with a multi-line
elif that decrements depth, replace "if depth < 0: balanced = False; break" with
a multi-line if that sets balanced = False and then breaks on separate lines,
and similarly replace "if char in '([{': d += 1" and "elif char in ')]}': d -=
1" with multi-line if/elif blocks; keep the same variable names (depth,
balanced, inside, d) and the control flow otherwise unchanged.
…ling for bytecode decompilation
Summary by CodeRabbit
New Features
Tests