Update ruff to 0.15.19, use it from crates.io#8160
Conversation
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_patma.py (TODO: 13) dependencies: dependent tests: (no tests depend on patma) [x] test: cpython/Lib/test/test_unpack.py dependencies: dependent tests: (no tests depend on unpack) [ ] test: cpython/Lib/test/test_syntax.py (TODO: 210) dependencies: dependent tests: (no tests depend on syntax) [ ] test: cpython/Lib/test/test_grammar.py (TODO: 11) dependencies: dependent tests: (no tests depend on grammar) [x] test: cpython/Lib/test/test_compile.py (TODO: 11) dependencies: dependent tests: (no tests depend on compile) 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 (1)
crates/codegen/src/compile.rs (1)
8271-8283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
keyonce and reuse it in this dict-comp path.
key.as_ref().expect(...)is repeated several times in the same flow. Bind it once and reuse it for compile/await/range operations.Proposed refactor
+ let key = key + .as_ref() + .expect("RustPython does not support PEP798 yet"); generators, &|compiler, collection_add_i| { // changed evaluation order for Py38 named expression PEP 572 - compiler.compile_expression( - &key.as_ref() - .expect("RustPython does not support PEP798 yet"), - )?; + compiler.compile_expression(key)?; compiler.compile_expression(value)?; compiler.set_source_range(TextRange::new( - key.as_ref() - .expect("RustPython does not support PEP798 yet") - .range() - .start(), + key.range().start(), value.range().end(), )); @@ - Self::contains_await( - &key.as_ref() - .expect("RustPython does not support PEP798 yet"), - ) || Self::contains_await(value) + Self::contains_await(key) || Self::contains_await(value) || Self::generators_contain_await(generators), *range, TextRange::new( - key.as_ref() - .expect("RustPython does not support PEP798 yet") - .range() - .start(), + key.range().start(), value.range().end(), ), - key.as_ref() - .expect("RustPython does not support PEP798 yet") - .range(), + key.range(), )?;As per coding guidelines, when logic is shared, extract the differing value first and call the common logic once to avoid duplication.
Also applies to: 8294-8309
🤖 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 `@crates/codegen/src/compile.rs` around lines 8271 - 8283, The dict-comp handling in compile.rs repeats key.as_ref().expect(...) multiple times; extract that value once in the affected flow and reuse it for compile_expression, any await handling, and source-range calculation. Update the dict-comp path around the existing compiler.compile_expression and compiler.set_source_range calls, and apply the same refactor to the other referenced block so the shared logic uses a single bound key value instead of duplicated expectations.Source: Coding guidelines
🤖 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 `@crates/codegen/src/unparse.rs`:
- Around line 256-260: The call in unparse_expr for the key handling path has a
redundant leading borrow that triggers a needless_borrow warning. Update the
expression in unparse.rs around the self.unparse_expr call so it passes the
reference returned by key.as_ref().expect("RustPython does not support PEP798
yet") directly, without adding an extra &.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 8271-8283: The dict-comp handling in compile.rs repeats
key.as_ref().expect(...) multiple times; extract that value once in the affected
flow and reuse it for compile_expression, any await handling, and source-range
calculation. Update the dict-comp path around the existing
compiler.compile_expression and compiler.set_source_range calls, and apply the
same refactor to the other referenced block so the shared logic uses a single
bound key value instead of duplicated expectations.
🪄 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: e4cf7347-2742-4018-9f90-8b6b99d09026
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockLib/test/test_grammar.pyis excluded by!Lib/**Lib/test/test_syntax.pyis excluded by!Lib/**
📒 Files selected for processing (16)
.github/dependabot.ymlCargo.tomlcrates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/codegen/src/unparse.rscrates/vm/Cargo.tomlcrates/vm/src/stdlib/_ast.rscrates/vm/src/stdlib/_ast/argument.rscrates/vm/src/stdlib/_ast/constant.rscrates/vm/src/stdlib/_ast/elif_else_clause.rscrates/vm/src/stdlib/_ast/module.rscrates/vm/src/stdlib/_ast/node.rscrates/vm/src/stdlib/_ast/parameter.rscrates/vm/src/stdlib/_ast/pattern.rscrates/vm/src/stdlib/_ast/string.rscrates/vm/src/stdlib/_ast/validate.rs
ShaharNaveh
force-pushed
the
ruff-0-15-19
branch
from
c179e29 to
2117417
Compare
ShaharNaveh
force-pushed
the
ruff-0-15-19
branch
from
2117417 to
6b8f4d0
Compare
|
I need patched version of ruff in #8138 |
I believe I could give a better answer after #8138 is merged. making this a draft for now |
Looking at it agian, it looks like we only add new fields to the AST structs but we don't really use them over at ruff side. wouldn't it be more sustainable for us to do: // In the RustPython crate, not in ruff
pub struct StmtDelete {
inner: ruff_python_ast::StmtDelete,
runtime_targets: Option<Vec<Option<crate::Expr>>>,
}we can add a clippy rule for disallowed_types to make sure we use our type only. I'm not so sure why is that patch needed anyways?how does cpython handle it? does they add those fields as well? |
|
cpython parser is more friendly to python runtime. ruff parser is more optimzied for ruff purpose. |
2 participants
Summary
astral-sh/ruff#26271
Summary by CodeRabbit
ThinVec.ThinVecsupport to the AST conversion layer.thin-vecworkspace dependency.