-
-
Notifications
You must be signed in to change notification settings - Fork 52
Centralize property creation. #667
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
68eaf8f
to
04071c9
Compare
As property creation is getting more complex, move all complexity into a sepatate builder class that handles everything.
04071c9
to
283247b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes property creation by introducing a dedicated PropertyBuilder that consolidates property instantiation and related logic. Key changes include:
- Updating constructor promotion and its corresponding tests to work with the new builder pattern.
- Replacing direct property instantiation with the PropertyBuilder in both regular and constructor-promotion contexts.
- Adjusting method calls (e.g., changing isProtected to isProtectedSet) and PHPUnit testing parameters as well as updating PHP CLI versions in the Makefile.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/integration/data/PHP8/ConstructorPromotion.php | Updates property promotion parameters and default values |
tests/integration/PHP8/ConstructorPromotionTest.php | Updates expected arguments and locations in test cases |
src/phpDocumentor/Reflection/Php/Factory/PropertyIterator.php | Updates method calls for async visibility checks |
src/phpDocumentor/Reflection/Php/Factory/PropertyBuilder.php | Introduces a new builder class for property creation |
src/phpDocumentor/Reflection/Php/Factory/Property.php | Refactors property creation to use the builder |
src/phpDocumentor/Reflection/Php/Factory/ConstructorPromotion.php | Refactors constructor promotion to use the builder |
Makefile | Upgrades PHP CLI versions for static analysis and updates test targets |
Comments suppressed due to low confidence (2)
tests/integration/PHP8/ConstructorPromotionTest.php:90
- [nitpick] The expected Location values have been updated. Please ensure that these new positions are consistently maintained in future changes to the code location extraction logic.
new Location(27, 517)
src/phpDocumentor/Reflection/Php/Factory/PropertyIterator.php:122
- Replacing isProtected() with isProtectedSet() ensures the async property checks are accurate. Double-check that similar method changes (like for private) are consistently applied throughout the codebase.
return $this->property->isPublicSet() || $this->property->isProtectedSet() || $this->property->isPrivateSet();
public string $name = 'default name', | ||
protected Email $email, | ||
private DateTimeImmutable $birth_date, | ||
) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The default value for the name property is now hardcoded. Verify that this change aligns with the new property creation strategy and is intentional across all usages.
public string $name = 'default name', | |
protected Email $email, | |
private DateTimeImmutable $birth_date, | |
) {} | |
public string $name = self::DEFAULT_NAME, | |
protected Email $email, | |
private DateTimeImmutable $birth_date, | |
) {} | |
private const DEFAULT_NAME = 'default name'; |
Copilot uses AI. Check for mistakes.
d3a3fc3
to
d1849f6
Compare
As property creation is getting more complex, move all complexity into a sepatate builder class that handles everything.