impl extract-superclass #2207#2310
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the Extract Superclass refactoring feature for Python code, allowing developers to extract selected class members into a new base class. This is part of the larger effort tracked in issue #2207 to expand Pyrefly's IDE refactoring capabilities.
Changes:
- Added
extract_superclass.rsimplementing the core refactoring logic to create a new base class, move selected members, and update inheritance relationships - Integrated the refactoring into the LSP code actions system
- Added comprehensive tests covering basic extraction and edge cases like metaclasses with pass insertion
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/state/lsp/quick_fixes/extract_superclass.rs |
New module implementing extract superclass refactoring logic including member selection, superclass creation, base class insertion, and cleanup |
pyrefly/lib/state/lsp/quick_fixes/mod.rs |
Module registration for extract_superclass |
pyrefly/lib/state/lsp.rs |
Transaction method to expose extract_superclass functionality |
pyrefly/lib/lsp/non_wasm/server.rs |
LSP server integration to make refactoring available as a code action |
pyrefly/lib/test/lsp/code_actions.rs |
Tests for basic extraction and metaclass edge case with proper pass insertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn is_member_stmt(stmt: &Stmt) -> bool { | ||
| matches!( | ||
| stmt, | ||
| Stmt::FunctionDef(_) | Stmt::ClassDef(_) | Stmt::Assign(_) | Stmt::AnnAssign(_) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The is_member_stmt function is duplicated in both extract_superclass.rs and move_members.rs (lines 189-194). Consider moving this function to extract_shared.rs to avoid code duplication and improve maintainability. This would ensure consistent behavior across different refactoring operations.
| fn generate_superclass_name(source: &str, class_name: &str) -> String { | ||
| let base = format!("{DEFAULT_SUPERCLASS_PREFIX}{class_name}"); | ||
| let mut counter = 1; | ||
| loop { | ||
| let candidate = if counter == 1 { | ||
| base.clone() | ||
| } else { | ||
| format!("{base}_{counter}") | ||
| }; | ||
| let needle = format!("class {candidate}"); | ||
| if !source.contains(&needle) { | ||
| return candidate; | ||
| } | ||
| counter += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The name generation logic could be refactored to use the existing unique_name helper from extract_shared.rs for better code reuse. While the current implementation works correctly, using the established helper would be more consistent with other refactoring operations. You could call it as: unique_name(&format!("{DEFAULT_SUPERCLASS_PREFIX}{class_name}"), |name| source.contains(&format!("class {name}")))
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
kinto0
left a comment
There was a problem hiding this comment.
LGTM. ill fix up duplicate code + add tests
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Summary
part of #2207
https://www.jetbrains.com/help/pycharm/extract-superclass.html
Added Extract Superclass refactor to create a new base class in the same file, move the selected members, update the original class bases, and insert pass when the original becomes empty.
Wired the new refactor into LSP code actions.
note: to keep it simple, it does not implement the “Make Abstract” yet.
Test Plan
Added tests.