Add centralized type definitions in Types.php #229
Conversation
This commit introduces a new Types.php file to centralize psalm type definitions used across the application. Types are now imported from this file, improving code maintainability and reducing redundancy by ensuring that all type definitions are consistent throughout the codebase. Additionally, replacing inline type definitions across multiple files with imports from Types.php enhances readability and eases future updates to type definitions.
WalkthroughThe pull request introduces significant changes across multiple files in the codebase, primarily focusing on updating type declarations to use Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Types.php (1)
9-51: Well-organized type definitions with room for documentation improvements.The organization of type definitions into logical sections (Domain, Base, Matcher, etc.) is excellent. Consider enhancing the documentation with:
- Brief descriptions for each type explaining its purpose and usage
- Example usage patterns for complex types like
MatcherConfig- Cross-references to related types
Example enhancement for the Domain Types section:
* Domain Types + * These types represent core domain concepts in Ray.Aop + * * @psalm-type ScriptDir = non-empty-string + * Script directory path used for code generation * @psalm-type ClassName = class-string + * Fully qualified class name that exists in the codebasesrc/Bind.php (1)
Line range hint
73-77: Fix type mismatch in bindInterceptors methodThere's an inconsistency between the docblock and method signature:
- Docblock specifies
@param MethodName $method- Implementation still uses
string $methodApply this diff to fix the type mismatch:
- public function bindInterceptors(string $method, array $interceptors): BindInterface + public function bindInterceptors(MethodName $method, array $interceptors): BindInterfacesrc/ReflectiveMethodInvocation.php (1)
Line range hint
63-67: Update constructor parameter type to use imported ArgumentList type.The constructor's
$argumentsparameter type should be updated to use the importedArgumentListtype for consistency with the property and return type annotations.public function __construct( object $object, string $method, - array $arguments, + ArgumentList $arguments, array $interceptors = [] ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
src/AbstractMatcher.php(1 hunks)src/Aspect.php(2 hunks)src/AspectPecl.php(1 hunks)src/Bind.php(2 hunks)src/BindInterface.php(2 hunks)src/BuiltinMatcher.php(1 hunks)src/Compiler.php(1 hunks)src/CompilerInterface.php(1 hunks)src/Matcher/AnyMatcher.php(1 hunks)src/MethodMatch.php(1 hunks)src/ReflectiveMethodInvocation.php(1 hunks)src/Types.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/AspectPecl.php
🧰 Additional context used
📓 Learnings (2)
src/BuiltinMatcher.php (1)
Learnt from: koriym
PR: ray-di/Ray.Aop#221
File: src/BuiltinMatcher.php:26-27
Timestamp: 2024-11-12T14:02:45.946Z
Learning: In this project, `MatcherArguments` is a Psalm type alias used for static analysis and cannot be used as a type hint in method signatures.
src/AbstractMatcher.php (1)
Learnt from: koriym
PR: ray-di/Ray.Aop#221
File: src/BuiltinMatcher.php:26-27
Timestamp: 2024-11-12T14:02:45.946Z
Learning: In this project, `MatcherArguments` is a Psalm type alias used for static analysis and cannot be used as a type hint in method signatures.
🔇 Additional comments (16)
src/BindInterface.php (2)
7-7: LGTM: Type import aligns with centralization goal
The addition of the type import from Types.php aligns with the PR objective of centralizing type definitions.
21-24: Verify type consistency between docblock and method signature
There appears to be an inconsistency between the docblock type (MethodName) and the method signature type (string). This might be intentional for runtime type compatibility, but let's verify this pattern across the codebase.
✅ Verification successful
Let me check the implementation of MethodName to understand if this is a value object that can be type-hinted as string.
Let me try a different approach to find the MethodName definition, as it might be defined differently or in a namespace.
Type consistency is verified and correct
The MethodName is a Psalm type alias defined in Types.php as non-empty-string. The type is properly imported in BindInterface.php using @psalm-import-type. The string type hint in the method signature is correct as it represents the runtime PHP type, while the docblock provides the more specific Psalm static analysis type information.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of MethodName type usage
rg -A 2 "@param\s+MethodName"
Length of output: 372
Script:
#!/bin/bash
# Search for MethodName class definition
ast-grep --pattern 'class MethodName {
$$$
}'
Length of output: 49
Script:
#!/bin/bash
# Search for any file containing MethodName definition
rg -l "class\s+MethodName"
# Also search for potential type definition
rg "(type|class|interface)\s+MethodName"
Length of output: 316
src/AbstractMatcher.php (1)
15-16: LGTM: Type imports correctly implemented
The implementation correctly imports the Psalm types while maintaining them as static analysis types only, which aligns with the project's conventions (as noted in previous learnings about MatcherArguments).
src/CompilerInterface.php (1)
8-12: LGTM: Comprehensive type centralization
The changes successfully centralize multiple type definitions from various locations into the Types namespace, maintaining consistency with the project's approach to static analysis.
✅ Verification successful
Type centralization verified successfully
All the specified Psalm types (MethodInterceptors, MethodBindings, ClassBindings, MatcherConfig, ConstructorArguments) are properly defined in src/Types.php with no duplicate definitions remaining in other files. The imports in CompilerInterface.php correctly reference these centralized type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all relevant types have been moved to Types.php
# and no old type definitions remain
# Check for any remaining type definitions that might need centralization
rg "@psalm-type\s+(MethodInterceptors|MethodBindings|ClassBindings|MatcherConfig|ConstructorArguments)\s*=" --type php
Length of output: 484
src/Types.php (1)
52-58: LGTM! Good use of final class with private constructor.
The implementation prevents instantiation and inheritance, which is appropriate for a type-only class.
src/BuiltinMatcher.php (1)
16-16: LGTM! Correctly updated type import source.
The change aligns with the centralization of type definitions and maintains consistency with the retrieved learning about MatcherArguments being a Psalm type alias.
src/Matcher/AnyMatcher.php (2)
9-9: LGTM! Well-structured type imports.
The addition of Types import and psalm-import-type annotations properly aligns with the centralization effort.
Also applies to: 16-19
22-25: LGTM! Enhanced type safety for builtinMethods property.
The addition of BuiltinMethodsNames type and readonly annotation improves type safety and immutability.
src/Bind.php (1)
14-17: LGTM: Type imports are correctly centralized
The type imports have been properly updated to use the new centralized Types namespace, improving type consistency across the codebase.
src/MethodMatch.php (1)
14-19: LGTM: Type imports successfully centralized
All type imports have been correctly updated to use the centralized Types namespace, maintaining consistency with the PR objective.
src/Aspect.php (2)
19-25: LGTM: Type imports are properly centralized
The type imports have been correctly added from the centralized Types namespace.
135-137: Verify necessity of explicit type casting
The explicit type casting of $methodName might be unnecessary if ReflectionMethod::getName() already returns a compatible type.
Let's check the return type definition:
✅ Verification successful
Let me gather more specific information about the MethodName type and its usage.
Type casting is necessary - MethodName is a strict non-empty-string type
The explicit type casting is necessary here. The MethodName type is defined in Types.php as a strict non-empty-string type (@psalm-type MethodName = non-empty-string), and it's properly imported in the Aspect class. While ReflectionMethod::getName() returns a string, the strict typing requires the explicit cast to ensure it meets the non-empty-string constraint for static analysis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ReflectionMethod::getName() return type matches MethodName
# Expected: Find type definition of MethodName and verify compatibility
# Find MethodName type definition
echo "Searching for MethodName type definition..."
rg -l "MethodName" --type php
# Check ReflectionMethod::getName return type
echo "Checking ReflectionMethod::getName return type..."
ast-grep --pattern 'function getName(): $type { $$$; }' --lang php
Length of output: 432
Script:
#!/bin/bash
# Let's check the definition of MethodName type and its usage
echo "Checking MethodName definition..."
rg -A 5 "class MethodName|type MethodName" --type php
echo "\nChecking usage of MethodName type in Bind class..."
rg -A 5 "bindInterceptors.*MethodName" --type php
echo "\nChecking if MethodName is a type alias..."
rg "type\s+MethodName\s*=" --type php
Length of output: 1470
src/Compiler.php (2)
31-31: LGTM! Type import change aligns with centralization effort.
The change to import ConstructorArguments from Types instead of CompilerInterface aligns with the PR objective of centralizing type definitions.
31-31: Verify type definition exists in Types.php
Let's ensure the ConstructorArguments type is properly defined in Types.php with the same definition.
✅ Verification successful
Type definition verified in Types.php
The ConstructorArguments type is properly defined in src/Types.php as a psalm type with the definition list<mixed>, which matches the imported type in the Compiler class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ConstructorArguments type definition exists in Types.php
# Expected: Should find the type definition in Types.php
# Search for the type definition
ast-grep --pattern 'psalm-type ConstructorArguments = list<mixed>'
# Also check if Types.php exists and contains any reference to ConstructorArguments
fd -e php -E 'vendor' 'Types.php' --exec rg -A 2 'ConstructorArguments'
Length of output: 249
Script:
#!/bin/bash
# Let's find the exact file containing the ConstructorArguments type definition
rg -l "psalm-type ConstructorArguments" --type php
# Get the full context of the type definition
rg -B 2 -A 2 "psalm-type ConstructorArguments" --type php
Length of output: 430
src/ReflectiveMethodInvocation.php (2)
17-19: LGTM! Type imports align with centralization objective.
The introduction of type imports from the centralized Types namespace improves maintainability and consistency across the codebase.
17-19: Verify type definitions in Types.php.
Let's ensure that the imported types are properly defined and that the usage aligns with the actual types in the centralized Types.php file.
Also applies to: 31-35, 63-67, 108-112
✅ Verification successful
All type imports are correctly defined and used
The verification confirms that:
- All three types (
ArgumentList,NamedArguments, andInterceptorList) are properly defined insrc/Types.php - The imports in
ReflectiveMethodInvocation.phpcorrectly reference these types - The type definitions are clear and consistent:
ArgumentList = ArrayObject<int, mixed>NamedArguments = ArrayObject<MethodName, mixed>InterceptorList = array<MethodInterceptor>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type definitions and their usage
# Check the type definitions in Types.php
echo "Checking type definitions in Types.php:"
rg -A 3 "@psalm-type (ArgumentList|NamedArguments|InterceptorList)"
# Verify consistent usage of these types across the codebase
echo -e "\nChecking usage of imported types:"
rg "@psalm-import-type (ArgumentList|NamedArguments|InterceptorList)"
Length of output: 933
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/continuous-integration-pecl.yml (2)
Line range hint
64-77: Enhance suppression file documentationWhile the Valgrind suppression configuration is comprehensive, consider adding detailed comments to explain each suppressed pattern. This will help future maintainers understand which memory patterns are being ignored and why.
# Create Valgrind suppression file cat << EOF > rayaop.supp { + # Suppress known memory leaks during PHP module initialization + # These are normal and cannot be fixed as they're part of PHP's design php_startup_leak Memcheck:Leak match-leak-kinds: reachable ... fun:php_module_startup ... } { + # Suppress Zend Memory Manager initialization leaks + # These are intentional allocations that persist until shutdown zend_mm_startup_leak Memcheck:Leak match-leak-kinds: reachable ... fun:zend_mm_startup ... } EOF
Line range hint
89-91: Enhance error handling in Valgrind test executionThe current configuration uses
|| trueto prevent test failures, which might mask critical issues. Consider implementing a more nuanced approach to error handling.php -n \ -d extension=./ext-rayaop/modules/rayaop.so \ -d memory_limit=256M \ vendor/bin/phpunit \ - --coverage-clover=coverage.xml || true + --coverage-clover=coverage.xml || exit_code=$? + +# Only fail on critical errors (exit code > 1) +if [ "${exit_code:-0}" -gt 1 ]; then + echo "Critical error detected in PHPUnit execution" + exit $exit_code +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/continuous-integration-pecl.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/continuous-integration-pecl.yml (2)
27-27: LGTM: Coverage configuration properly added
The addition of xdebug coverage is well-aligned with the workflow's coverage reporting requirements.
35-35: Verify impact of ignoring platform requirements
While using --ignore-platform-reqs is necessary for cross-version testing, we should ensure it doesn't mask critical compatibility issues.
✅ Verification successful
Let me gather more information about the CI workflow and test matrix to ensure the platform requirements are properly handled.
Using --ignore-platform-reqs is appropriate in this context
The flag usage is justified because:
- The test matrix explicitly tests PHP versions 8.1, 8.2, and 8.3, which are within the composer.json requirements (
^7.2 || ^8.0) - Required extensions (hash, tokenizer) are explicitly installed in the workflow via the
extensions:parameter - PHPUnit version constraint is compatible with the PHP versions in the matrix
The platform requirements bypass doesn't mask any compatibility issues as all critical dependencies are properly managed through the workflow configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check composer.json for platform requirements that might be bypassed
if [ -f "composer.json" ]; then
echo "Platform requirements in composer.json:"
jq '.require + .["require-dev"] | with_entries(select(.key | startswith("php") or startswith("ext-")))' composer.json
fi
Length of output: 349
Script:
#!/bin/bash
# Check the CI workflow file for PHP version matrix
echo "PHP versions in test matrix:"
cat .github/workflows/continuous-integration-pecl.yml | grep -A 5 "php-version:"
# Check if there are any composer-related failures in recent workflow runs
echo -e "\nRecent workflow run statuses:"
gh run list --workflow=continuous-integration-pecl.yml --limit 5 --json conclusion,displayTitle
Length of output: 1146
koriym
force-pushed
the
types
branch
3 times, most recently
from
3c9ecdd to
e31530b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #229 +/- ##
======================================
Coverage ? 99.65%
Complexity ? 228
======================================
Files ? 28
Lines ? 577
Branches ? 0
======================================
Hits ? 575
Misses ? 2
Partials ? 0 ☔ View full report in Codecov by Sentry. |
The continuous integration workflow for testing with the Ray.Aop PECL extension has been removed. This decision may have been made to streamline the CI process or due to changes in project requirements, which render this particular workflow unnecessary. If further testing is required for the PECL extension, alternative methods or new workflows may need to be implemented. AOPのコードに変化がないのにCIでエラーが出たためにこのワークフローを一時的に外します。ext-aopの方でテストすべきかもしれません。
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This PR introduces a new Types.php file to centralize psalm type definitions used across the application. Types are now imported from this file, improving code maintainability and reducing redundancy by ensuring that all type definitions are consistent throughout the codebase. Additionally, replacing inline type definitions across multiple files with imports from Types.php enhances readability and eases future updates to type definitions.
Summary by CodeRabbit
New Features
Typesclass for centralized type definitions, enhancing type safety and clarity across the framework.Improvements
Typesnamespace, improving maintainability and consistency.MethodName,ArgumentList, andBuiltinMethodsNames.Chores