feat: add removeClassAttribute method for class attributes#58
feat: add removeClassAttribute method for class attributes#58artengin wants to merge 10 commits into
Conversation
Coverage Report for CI Build 25501877436Coverage increased (+0.06%) to 98.615%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds support for removing PHP 8 attributes from a specific class declaration via PHPFileBuilder, including coverage for grouped (#[A, B]) and repeated attributes, plus a small refactor of the visitor base class to centralize “parent node not found” handling.
Changes:
- Introduce
RemoveClassAttributevisitor and expose it viaPHPFileBuilder::removeClassAttribute(). - Move
hasParentNode+afterTraverse()“parent node not found” hook intoBaseNodeVisitorAbstract(simplifyingInsertOrUpdateNodeAbstractVisitor). - Add PHPUnit cases + fixtures for removal, grouped attributes, no-op when attribute missing, and error scenarios.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Builders/PHPFileBuilder.php |
Adds removeClassAttribute() API to attach the new visitor. |
src/Visitors/RemoveClassAttribute.php |
Implements class-level attribute removal logic. |
src/Visitors/BaseNodeVisitorAbstract.php |
Centralizes hasParentNode and the post-traverse hook. |
src/Visitors/InsertOrUpdateNodeAbstractVisitor.php |
Removes duplicated hasParentNode/afterTraverse() logic now handled in the base class. |
tests/PHPFileBuilderTest.php |
Adds test coverage for removeClassAttribute behavior and errors. |
tests/fixtures/PHPFileBuilderTest/original/class_with_attributes.php |
New input fixture for class-level attributes + member attributes. |
tests/fixtures/PHPFileBuilderTest/original/class_with_grouped_attributes.php |
New input fixture for #[A, B] grouped attributes. |
tests/fixtures/PHPFileBuilderTest/original/class_with_multiple_same_attributes.php |
New input fixture for repeated same-name class attributes. |
tests/fixtures/PHPFileBuilderTest/results/class_without_attributes.php |
Expected output after removing class-level attributes. |
tests/fixtures/PHPFileBuilderTest/results/class_with_grouped_attributes_one_removed.php |
Expected output after removing one attribute from a grouped block. |
tests/fixtures/PHPFileBuilderTest/results/class_with_attributes_unchanged.php |
Expected output for the no-op (no matching attribute). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected function validateClassName(Class_ $node): void | ||
| { | ||
| if ($this->className !== $node->name->name) { | ||
| throw new NodeNotExistException('Class', $this->className); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateClassName() throws NodeNotExistException as soon as it encounters a Class_ node whose name doesn’t match the target. If the file contains multiple classes, this will abort on the first non-target class even if the requested class exists later in the AST. Consider instead tracking a hasTargetClass flag: on non-matching classes, just return without changes; on match, set the flag and perform removal; then in afterTraverse() throw NodeNotExistException('Class', $className) only if no matching class was ever found (while still using parentNodeNotFoundHook() for the “no class nodes at all” case).
There was a problem hiding this comment.
Multiple classes in a single file violates PSR-1
|
@codex review |
![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: eb2719b287
ℹ️ 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".
| if ($this->className !== $node->name->name) { | ||
| throw new NodeNotExistException('Class', $this->className); |
There was a problem hiding this comment.
validateClassName() throws on the first Class_ node whose name differs, so removeClassAttribute() fails in files that contain any additional class node (for example, a second top-level class or an anonymous class encountered before/after the target) even when the requested $className is present. In the anonymous-class case, $node->name is null, so $node->name->name can also crash at runtime. The visitor should track whether a matching named class was found and only throw NodeNotExistException after traversal if none matched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Multiple classes in a single file violates PSR-1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c77fe6b11
ℹ️ 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".
|
|
||
| protected function validateClassName(Class_ $node): void | ||
| { | ||
| if ($this->className !== $node->name->name) { |
There was a problem hiding this comment.
Fresh evidence: even in a PSR-1 single-class file, a nested new class {} creates a Class_ node with no name, and this comparison dereferences $node->name unconditionally. When removeClassAttribute() traverses such a file, it fails before reaching the intended class (or throws NodeNotExistException on the wrong node), so attribute removal is impossible for valid PHP files that include anonymous classes.
Useful? React with 👍 / 👎.
refs: #57