Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: add support for Custom Skip Voters #5002

Conversation

kaczenski
Copy link
Contributor

Description:

This pull request refactors the constructor of the Skipper class to improve its compatibility with dependency injection and to support custom SkipVoter implementations.

Changes Made:

  1. Changed the constructor to accept an iterable of SkipVoterInterface objects, allowing for more flexibility in injecting multiple skip voters without having to explicitly list them.

  2. Added a type hint for the $skipVoters parameter to ensure that all elements within the iterable are instances of SkipVoterInterface.

  3. Used the Assert::allIsInstanceOf method to validate that all elements in $skipVoters are indeed instances of SkipVoterInterface.

Reasoning:

In version 0.16.0, before the introduction of the new Lazy Container, I implemented a custom SkipVoter for the company I work for. This custom implementation worked seamlessly. However, with the introduction of the Lazy Container and the subsequent change in the constructor signature, the ability to use custom SkipVoter implementations was hindered.

By allowing an iterable of skip voters, we can easily extend or modify the list of skip voters without changing the constructor signature. This change also aligns better with dependency injection principles, making it easier to bind and inject dependencies.

Furthermore, I plan to submit another pull request proposing a new "collector" based skip voter. The idea behind this is to provide a configurable mechanism to skip classes based on specific criteria, such as implementing a certain interface or extending a specific class.

Potential Concerns:

  1. Backward Compatibility: This change might affect parts of the codebase or external projects that rely on the old constructor signature.

  2. Performance: Using an iterable and asserting all elements might introduce a slight performance overhead. The impact should be minimal.

3. Original Design Intent: I'd love to understand the original intention behind the design change. The previous design might have been intentional to restrict the number or type of skip voters. It's essential to ensure that this change aligns with the project's goals.

@TomasVotruba
Copy link
Member

Thanks for proposal. The reasoning is clear and feature well implemented 👍

There are 2 details to resolve and then we're good to go

@TomasVotruba
Copy link
Member

Thank you @kaczenski 👍

@TomasVotruba TomasVotruba merged commit ef003b2 into rectorphp:main Sep 12, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants