Skip to content

Revert "Use ruff for Expr unparsing" #6206

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.

Sign up for GitHub

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

Jump to bottom
Merged
youknowone merged 1 commit into from
Oct 22, 2025
Merged

Revert "Use ruff for Expr unparsing" #6206

youknowone merged 1 commit into from
Oct 22, 2025

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Oct 22, 2025 •
edited by coderabbitai bot
Loading

Reverts #6124

See #6124 (comment)

Summary by CodeRabbit

  • Chores
    • Optimized internal dependencies and refactored code organization for improved build efficiency.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025 •
edited
Loading

Walkthrough

This PR removes workspace and package dependencies on external ruff crates (ruff_python_codegen and ruff_source_file), replaces a local unparse_expr helper with an external UnparseExpr implementation imported from a new internal unparse module, and cleans up unused imports.

Changes

Cohort / File(s) Summary
Workspace dependency cleanup
Cargo.toml		 Removed ruff_python_codegen from workspace dependencies
Package dependency pruning
compiler/codegen/Cargo.toml		 Removed ruff_python_parser, ruff_python_codegen, and ruff_source_file from dependencies
Codegen unparse refactoring
compiler/codegen/src/compile.rs		 Removed private unparse_expr helper function; added UnparseExpr import; replaced unparse_expr calls with UnparseExpr::new().to_string() at two call sites; removed unused LineEnding import
Module structure
compiler/codegen/src/lib.rs		 Added private unparse module declaration

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes involve heterogeneous edits across dependency manifests and source code refactoring. While individual changes are straightforward, understanding the rationale for dependency removal and verifying that the UnparseExpr replacement maintains functional equivalence across both call sites requires moderate attention.

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A codegen hop, dependencies drop,
unparse_expr we bid adieu,
UnparseExpr now unpacks the view,
cleaner modules help us skip,
workspace pruned, refactored trip! 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Revert "Use ruff for Expr unparsing"" directly and accurately describes the main change in the changeset. The modifications across multiple files—including removal of ruff-related dependencies (ruff_python_codegen, ruff_python_parser, ruff_source_file) from Cargo.toml files and refactoring of the expression unparsing logic in compile.rs—are all consistent with reverting a previous change that integrated ruff for expression unparsing. The title is concise, specific, and clearly communicates the primary intent to a developer scanning the history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f22aed2 and de5d207.

⛔ Files ignored due to path filters (12)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_future_stmt/badsyntax_future10.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future3.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future4.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future5.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future6.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future7.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future8.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/badsyntax_future9.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/future_test1.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/future_test2.py is excluded by !Lib/**
  • Lib/test/test_future_stmt/test_future.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • Cargo.toml (0 hunks)
  • compiler/codegen/Cargo.toml (0 hunks)
  • compiler/codegen/src/compile.rs (3 hunks)
  • compiler/codegen/src/lib.rs (1 hunks)
💤 Files with no reviewable changes (2)
  • Cargo.toml
  • compiler/codegen/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

  • compiler/codegen/src/lib.rs
  • compiler/codegen/src/compile.rs
🧬 Code graph analysis (2)
compiler/codegen/src/lib.rs (1)
Lib/ast.py (1)
  • unparse (1815-1817)
compiler/codegen/src/compile.rs (2)
compiler/codegen/src/unparse.rs (2)
  • new (34-36)
  • new (637-639)
compiler/codegen/src/symboltable.rs (3)
  • new (61-74)
  • new (175-182)
  • new (630-640)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (4)
compiler/codegen/src/lib.rs (1)

16-16: LGTM! Module declaration is correctly placed.

The private unparse module declaration follows Rust conventions and integrates cleanly with the existing module structure.

compiler/codegen/src/compile.rs (3)

17-17: LGTM! Import correctly added.

The UnparseExpr import from the internal unparse module is properly integrated into the existing crate-local imports.

3612-3612: LGTM! Correct usage for mapping pattern keys.

The unparsing of literal keys is correctly implemented. The temporary UnparseExpr is immediately converted to a string with no lifetime concerns, and the usage is properly guarded by the literal type check at line 3610.

4166-4169: LGTM! Correct implementation for future annotations.

The unparsing of annotations when future_annotations is enabled is correctly implemented. The pattern is safe and aligns with PEP 563 requirements to store annotations as strings. The temporary UnparseExpr is immediately converted with no lifetime issues.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone merged commit 153d0ee into RustPython:main Oct 22, 2025
12 checks passed
@ShaharNaveh ShaharNaveh mentioned this pull request Oct 22, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShaharNaveh @youknowone