refactor: replace abstract visitor hierarchy with contract-based polymorphic dispatch#67
refactor: replace abstract visitor hierarchy with contract-based polymorphic dispatch#67artengin wants to merge 10 commits into
Conversation
|
@codex review |
|
It inherits from this commit: #51 |
Coverage Report for CI Build 27781967069Coverage decreased (-0.08%) to 98.611%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
![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: d13ce08389
ℹ️ 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".
| fn ($trait) => new TraitUse([new Name(class_basename($trait))]), | ||
| array_unique(array_filter($this->traits)), |
There was a problem hiding this comment.
getInsertableNodes() now calls array_unique() before class_basename(), so inputs like ['App\\Traits\\Loggable', 'Loggable'] produce two identical TraitUse nodes. filterExistingNodes() only removes traits already present in the file, not duplicates within the new candidate list, so this can emit duplicate use Loggable; entries and generate invalid class composition when the same trait is applied twice.
Useful? React with 👍 / 👎.
DenTray
left a comment
DenTray
left a comment
There was a problem hiding this comment.
@artengin let's make another way to code optimization. Instead of implementing new contract let's change the addImports and addTraits methods logic to add the same visitor N times. It should simplify architecture and AddImports/AddTraits visitors logic and
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e18bad878
ℹ️ 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".
| foreach ($imports as $import) { | ||
| $this->traverser->addVisitor(new AddImport($import)); |
There was a problem hiding this comment.
addImports() now forwards each raw array element directly into new AddImport($import), but AddImport::__construct requires a string and no longer benefits from the old filter() step. If callers pass optional values (for example null or empty strings from config/merge pipelines), this now throws a TypeError (for null) or can emit an invalid use ; statement (for empty string), where the previous implementation silently skipped those values.
Useful? React with 👍 / 👎.
| foreach ($traits as $trait) { | ||
| $this->traverser->addVisitor(new AddTrait($trait)); |
There was a problem hiding this comment.
addTraits() also removed the prior filter() behavior and now instantiates AddTrait for every raw element. Passing nullable/empty trait entries (common when composing arrays conditionally) now fails at runtime (TypeError for null) or may build malformed trait/use nodes for empty strings, instead of being ignored as before, so this refactor introduces a backward-incompatible failure mode.
Useful? React with 👍 / 👎.
|
@artengin please resolve conflicts |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18ad99c1d7
ℹ️ 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".
| { | ||
| $this->traverser->addVisitor(new AddImports($imports)); | ||
| foreach ($imports as $import) { | ||
| $this->traverser->addVisitor(new AddImport($import)); |
There was a problem hiding this comment.
When callers pass multiple new imports, this now enqueues one AddImport visitor per item, but the project’s NodeTraverser::traverse() reverses the visitor list before traversal. Since these imports are inserted from AddImport::afterTraverse(), they are applied last-to-first, so addImports([SecondClass, ThirdClass]) emits ThirdClass before SecondClass (and addTraits() inherits the same reversed import order), breaking the previously preserved caller order and the existing fixture expectations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee2921474
ℹ️ 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".
| { | ||
| $this->traverser->addVisitor(new AddTraits($traits)); | ||
| foreach ($traits as $trait) { | ||
| $this->traverser->addVisitor(new AddTrait($trait)); |
There was a problem hiding this comment.
When addTraits() receives multiple new traits for a class/trait/enum with no existing trait uses, this loop registers one AddTrait visitor per trait, but NodeTraverser::traverse() reverses the visitor list before traversal. Since each AddTrait inserts during leaveNode, addTraits([FirstTrait, SecondTrait, ThirdTrait]) emits the trait-use statements as ThirdTrait, SecondTrait, FirstTrait, rather than the caller order expected by the existing add_traits_to_enum.php/add_traits_to_trait.php fixtures.
Useful? React with 👍 / 👎.
3 participants
No description provided.