feat: ability remove method - #72
Conversation
Coverage Report for CI Build 27606062657Coverage increased (+0.02%) to 98.691%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f9eaf252
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Implements a new PHPFileBuilder::removeMethod() feature intended to remove a method by name from a PHP class-like structure (per issue #70).
Changes:
- Added
PHPFileBuilder::removeMethod(string $methodName)API. - Introduced a new AST visitor (
RemoveNode) to remove nodes by name/type. - Added a PHPUnit test + fixture for removing a method from a class, and documented the new feature in README.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Builders/PHPFileBuilder.php |
Adds the new removeMethod() public API. |
src/Visitors/RemoveNode.php |
Introduces a generic visitor to remove a node by name/type. |
tests/PHPFileBuilderTest.php |
Adds a test for removing a method from a class fixture. |
tests/fixtures/PHPFileBuilderTest/class_method_removed.php |
Expected output fixture after method removal. |
README.md |
Documents the new removeMethod feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a41536f40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6279f4f726
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53cdf74912
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
artengin
left a comment
artengin
left a comment
There was a problem hiding this comment.
In the Contracts section, RemoveNodeContract is missing. Please add it alongside the existing two contracts:
RemoveNodeContract — The visitor can remove an existing node. Requires shouldRemoveNode(Node): bool. The base class iterates over child statements and removes the first matching node.
Also update step 3 in Creating a New Visitor:
Implement InsertNodeContract, UpdateNodeContract, RemoveNodeContract, or a combination.
|
|
||
| class RemoveMethod extends AbstractNodeVisitor implements RemoveNodeContract | ||
| { | ||
| protected array $allowedParentNodesTypes = self::ANY_TYPE; |
There was a problem hiding this comment.
Replace ANY_TYPE with an explicit list (Class_, Interface_, Trait_, Enum_) so the visitor only removes ClassMethod nodes inside class-like structures.
There was a problem hiding this comment.
I think is any strange, this parameter not effected to logic in AbstractNodeVisitorWe just find the class method is and delete it, if is't classMethod we skip it.
Hm.... Maybe we can find a way to not have to declare this parameter in all child classes?
There was a problem hiding this comment.
The restriction is mainly to ensure the visitor operates only on relevant file types. For RemoveMethod, passing an incorrect file type wouldn't cause any issues, the method just wouldn't be found and removed.
Since we're specifically looking for ClassMethod nodes, I felt it was reasonable to limit the visitor to class-like structures where such nodes can actually exist (Class, Interface, Trait, and Enum).
There was a problem hiding this comment.
@artengin
Please look at new version. I think we can add new abstraction for remove methods, because its clear and not touch other files a lot. And we can easily add removers without touch in defferent abstractions.
Class, Interface, Trait, and Enum in this version not checked in sepparate method, we have will check by package abstraction ClassMethod and this case cover our all requipments from task
You also look at version with sepparate contracts and modify AbstractNodeVisitor
FYI @DenTray (please put feedback also)
There was a problem hiding this comment.
I propose going with the contract-based approach for the remove operation, as it aligns with the current project style.
In the future, a single visitor will be able to use the remove contract alongside other contracts simultaneously, though I don't have a concrete use case for that combination yet.
9b57280#diff-ab3ffc8a3d6f694b09de81a8e10ca06ca24389fd6f5d7c63ca813600babb85c4
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0027a746e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efd8d5e6fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2670a9e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
# Conflicts: # README.md # tests/PHPFileBuilderTest.php
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af2c8ff25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9ed253f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return $node instanceof ClassMethod | ||
| && $node->name->toString() === $this->methodName; |
There was a problem hiding this comment.
When a file contains an anonymous class or another class-like declaration with a method of the same name, this predicate matches every ClassMethod visited anywhere in the AST, so removeMethod('foo') removes those nested/secondary methods as well as the intended method. Existing method visitors operate through the parent class/trait/enum body, but this visitor bypasses that scoping by matching raw ClassMethod nodes globally; constrain removal to the class-like node being modified (or at least skip anonymous/nested classes) to avoid deleting unrelated methods.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
refs: #70