Support enum in attribute arguments #212
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
tests/Fake/Annotation/FakeMarker5.php (1)
17-17: Consider renaming the constructor parameter fromfruitstofruitto accurately reflect that it accepts a single enum value.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/MethodSignatureString.php (2 hunks)
- tests/AopCodeTest.php (2 hunks)
- tests/Fake/Annotation/FakeMarker5.php (1 hunks)
- tests/Fake/FakePhp81Enum.php (1 hunks)
- tests/Fake/FakePhp8Types.php (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/Fake/FakePhp81Enum.php
Additional comments not posted (5)
tests/Fake/FakePhp8Types.php (2)
8-8: Correctly added use statement forFakeMarker5.
66-67: Properly annotatedmethod23withFakeMarker5usingFakePhp81Enum::Apple.src/MethodSignatureString.php (1)
10-10: Correctly added use statement forUnitEnum.tests/AopCodeTest.php (2)
52-52: Correctly updated the loop condition to includemethod23.
90-91: Properly added a new assertion formethod23with theFakeMarker5annotation.
src/MethodSignatureString.php
Outdated
| if ($value instanceof UnitEnum) { | ||
| $formattedValue = preg_replace('/\s+/', ' ', '\\' . var_export($value, true)); | ||
| $argRepresentation = is_numeric($name) ? $formattedValue : "{$name}: {$formattedValue}"; | ||
| $formattedArgs[] = $argRepresentation; | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Regex Pattern for Formatting UnitEnum Values Needs Adjustment
The current regex pattern does not fully handle leading and trailing spaces in UnitEnum values. The test cases reveal that leading and trailing spaces are not being removed as expected.
- Line 58: Adjust the regex pattern to ensure it removes leading and trailing spaces effectively.
Analysis chain
Proper handling of UnitEnum instances in attribute arguments. Ensure the regex pattern in line 58 is thoroughly tested.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the regex pattern used for formatting `UnitEnum` values.
# Test: Check if the regex pattern correctly formats enum values without extra spaces.
echo "UnitEnum::Apple" | grep -P '^\s+|\\s+$'
Length of output: 45
Script:
#!/bin/bash
# Description: Verify the regex pattern used for formatting `UnitEnum` values.
# Test: Check if the regex pattern correctly formats enum values without extra spaces.
# We will test with multiple cases including leading, trailing, and multiple spaces.
test_cases=(
"UnitEnum::Apple"
" UnitEnum::Apple"
"UnitEnum::Apple "
" UnitEnum::Apple "
"UnitEnum:: Apple"
"UnitEnum::Apple "
"UnitEnum:: Apple"
)
for test_case in "${test_cases[@]}"; do
echo "Original: '$test_case'"
formatted_value=$(echo "$test_case" | sed 's/\s\+/ /g')
echo "Formatted: '$formatted_value'"
echo
done
Length of output: 2097
There was a problem hiding this comment.
@coderabbitai Is this still valid?
There was a problem hiding this comment.
@koriym The previous concern about the regex pattern not handling leading and trailing spaces is still valid. The script output confirms that the current pattern (sed 's/^\s+|\s+$//g') did not effectively remove leading and trailing spaces.
To correctly handle this, you should use the following regex pattern:
sed 's/^\s\+//;s/\s\+$//'This pattern will remove spaces at the beginning and end of the string.
Would you like me to update the code with this corrected pattern?
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the regex pattern used for formatting `UnitEnum` values.
# Test: Check if the regex pattern correctly formats enum values without extra spaces.
# We will test with multiple cases including leading, trailing, and multiple spaces.
test_cases=(
"UnitEnum::Apple"
" UnitEnum::Apple"
"UnitEnum::Apple "
" UnitEnum::Apple "
"UnitEnum:: Apple"
"UnitEnum::Apple "
"UnitEnum:: Apple"
)
for test_case in "${test_cases[@]}"; do
echo "Original: '$test_case'"
formatted_value=$(echo "$test_case" | sed 's/^\s+|\s+$//g')
echo "Formatted: '$formatted_value'"
echo
done
Length of output: 2131
New Learnings Added
User: koriym
PR: ray-di/Ray.Aop#212
File: src/MethodSignatureString.php:0-0
Timestamp: 2024-05-20T02:13:49.359Z
Learning: The regex pattern `sed 's/^\s+|\s+$//g'` does not effectively remove leading and trailing spaces. The correct pattern to use is `sed 's/^\s\+//;s/\s\+$//'`.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/MethodSignatureString.php (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/MethodSignatureString.php
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/MethodSignatureString.php (3 hunks)
- tests/AopCodeTest.php (2 hunks)
- tests/Fake/Annotation/FakeMarker5.php (1 hunks)
- tests/Fake/Annotation/FakeMarker6.php (1 hunks)
- tests/Fake/FakePhp8Types.php (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/Fake/Annotation/FakeMarker5.php
Additional Context Used
GitHub Check Runs (1)
sa / PHPStan failure (3)
src/MethodSignatureString.php: [failure] 105-105:
Method Ray\Aop\MethodSignatureString::representArg() should return string but returns string|null.
Additional comments not posted (4)
tests/Fake/Annotation/FakeMarker6.php (1)
1-22: The implementation ofFakeMarker6with enum parameters aligns well with the PR's objectives.tests/Fake/FakePhp8Types.php (1)
8-9: Correct implementation of new annotations using enums inFakePhp8Types.Also applies to: 67-71
src/MethodSignatureString.php (1)
10-10: Proper inclusion ofUnitEnumto support enum types in method signatures.tests/AopCodeTest.php (1)
52-52: Correct extension of loop condition to include testing formethod24.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/MethodSignatureString.php (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/MethodSignatureString.php
|
@koriym ご確認をよろしくお願いいたします。 |
Simplified the representArg method logic for handling UnitEnum instances. The refactoring involved removing duplicate code to improve readability and maintainability.
Replaced magic values in MethodSignatureString class with class constants for better readability and maintainability. This includes PHP version check and the indent string.
The method signature string generation was refactored for improved readability and maintainability. Separate helper methods were created to handle the generation of doc comments, attributes, access modifiers, and the method signature itself. This reduces the complexity and length of the main 'get' method.
The SuppressWarnings PHPMD.CyclomaticComplexity annotation used on the MethodSignatureString class was not necessary and has now been removed. The removal of this annotation will ensure that the cyclomatic complexity app metric is appropriately analyzed and followed for this class.
The generateParameterCode method in MethodSignatureString.php has been changed from public to private for encapsulation purposes. This change increases the integrity of the data and ensures that
The return type of the representArg function in MethodSignatureString.php was updated. The previous nullable string type has been replaced with a non-nullable string type to ensure it always returns a string.
The method in the MethodSignatureString class has been renamed from "representArg" to "formatArg" for better understanding of its function. This change improves readability and provides clearer naming convention in the codebase.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #212 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 189 196 +7
===========================================
Files 26 26
Lines 503 512 +9
===========================================
+ Hits 503 512 +9 ☔ View full report in Codecov by Sentry. |
koriym
force-pushed
the
enum-in-attribute-args
branch
from
e1b5d19
to
0007969
Compare
Update MethodSignatureString to enhance argument formatting This commit updates the formatArg function in MethodSignatureString to ensure strict string output. It also removes unnecessary string casting in the foreach loop that processes argument lists. The changes provide a more consistent and reliable argument format for use in function signatures.
koriym
force-pushed
the
enum-in-attribute-args
branch
from
0007969
to
74dd135
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/MethodSignatureString.php (3 hunks)
Additional comments not posted (10)
src/MethodSignatureString.php (10)
10-10: Addeduse UnitEnum;to support enum types in attribute arguments.
25-28: Introduced constants to manage PHP version differences and indentation.
35-35: Utilization of constants for determining nullable string based on PHP version.
45-50: Refactoredgetmethod to use helper methods for building method signatures.
54-59: MethodgetDocCommentsimplifies the extraction of doc comments.
62-84: Enhanced attribute handling inaddAttributesmethod, including support for PHP 8.
87-92: MethodaddAccessModifierscorrectly adds method access modifiers to the signature.
95-107: MethodaddMethodSignatureconstructs the method signature using parameter and return type information.
113-119: IntroducedformatArgmethod to handleUnitEnumvalues differently, ensuring proper string representation.
122-122:generateParameterCodemethod efficiently constructs parameter strings with type, default value, and other attributes.
|
@apple-x-co 少しリファクタリングするつもりが大きなものになってしまいました>< |
|
@coderabbitai review |
Actions PerformedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/MethodSignatureString.php (3 hunks)
- tests/AopCodeTest.php (2 hunks)
- tests/Fake/Annotation/FakeMarker5.php (1 hunks)
- tests/Fake/Annotation/FakeMarker6.php (1 hunks)
- tests/Fake/FakePhp81Enum.php (1 hunks)
- tests/Fake/FakePhp8Types.php (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/MethodSignatureString.php
- tests/AopCodeTest.php
- tests/Fake/Annotation/FakeMarker5.php
- tests/Fake/Annotation/FakeMarker6.php
- tests/Fake/FakePhp81Enum.php
- tests/Fake/FakePhp8Types.php
koriym
force-pushed
the
enum-in-attribute-args
branch
from
632473a
to
b609f43
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/MethodSignatureString.php (2 hunks)
- tests/AopCodeTest.php (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/AopCodeTest.php
Additional Context Used
GitHub Check Runs (2)
sa / Psalm failure (7)
src/MethodSignatureString.php: [failure] 68-68:
ImpureMethodCall: Cannot call a possibly-mutating method ReflectionMethod::getAttributes from a mutation-free context
src/MethodSignatureString.php: [failure] 69-69:
ImpureMethodCall: Cannot call a possibly-mutating method ReflectionAttribute::getArguments from a mutation-free context
src/MethodSignatureString.php: [failure] 103-103:
ImpureMethodCall: Cannot call a possibly-mutating method Ray\Aop\TypeString::__invoke from a mutation-free context
src/MethodSignatureString.php: [failure] 123-123:
ImpureMethodCall: Cannot call a possibly-mutating method Ray\Aop\TypeString::__invoke from a mutation-free contextcs / Coding Standards failure (3)
src/MethodSignatureString.php: [failure] 23-23:
Found multi-line doc comment with single line content, use one-line doc comment instead.
Additional comments not posted (4)
src/MethodSignatureString.php (4)
28-31: Constants for PHP version and nullable strings have been added.These constants improve readability and maintainability by centralizing version-specific behavior.
38-38: Ensure the constructor's logic correctly handles PHP version comparisons.Verification successful
The constructor logic in
MethodSignatureStringcorrectly handles PHP version comparisons by setting$nullableStrbased on the provided$phpVersion. The comparison logic is straightforward and appears to be implemented correctly.
- The constructor checks if
$phpVersionis greater than or equal toself::PHP_VERSION_8.- If true, it sets
$nullableStrtoself::NULLABLE_PHP8.- Otherwise, it sets
$nullableStrtoself::NULLABLE_PHP7.This logic ensures that the correct nullable string is used based on the PHP version.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the PHP version comparison logic correctly initializes `nullableStr`. # Test: Check if the nullable string is correctly set based on PHP version. php -r 'echo (80000 >= 80000 ? "null|" : "?");'Length of output: 120
Script:
#!/bin/bash # Description: Verify the constructor's logic for handling PHP version comparisons in MethodSignatureString. # Locate the MethodSignatureString class and its constructor ast-grep --lang php --pattern $'class MethodSignatureString { $$$ public function __construct($_) { $$$ } $$$ }'Length of output: 232
Script:
#!/bin/bash # Description: Verify the constructor's logic for handling PHP version comparisons in MethodSignatureString. # Locate the MethodSignatureString class and its constructor rg 'class MethodSignatureString' -A 20 | rg 'public function __construct'Length of output: 154
Script:
#!/bin/bash # Description: Extract the full constructor implementation of MethodSignatureString to verify PHP version comparison logic. # Extract the constructor implementation from MethodSignatureString class rg 'public function __construct' -A 20 src/MethodSignatureString.phpLength of output: 802
44-50: Refactoredgetmethod to use helper methods for building method signatures.This refactoring enhances modularity and readability.
112-118: HandleUnitEnumvalues in attribute arguments.The method
formatArgcorrectly formats enum values, enhancing the flexibility of attribute annotations.
Removed unnecessary spaces in array definitions in tests and updated the method of handling whitespace in the MethodSignatureString. This change enhances code readability by ensuring consistent formatting standards across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/MethodSignatureString.php (2 hunks)
- tests/AopCodeTest.php (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/AopCodeTest.php
Additional Context Used
GitHub Check Runs (1)
sa / Psalm failure (7)
src/MethodSignatureString.php: [failure] 66-66:
ImpureMethodCall: Cannot call a possibly-mutating method ReflectionMethod::getAttributes from a mutation-free context
src/MethodSignatureString.php: [failure] 67-67:
ImpureMethodCall: Cannot call a possibly-mutating method ReflectionAttribute::getArguments from a mutation-free context
src/MethodSignatureString.php: [failure] 101-101:
ImpureMethodCall: Cannot call a possibly-mutating method Ray\Aop\TypeString::__invoke from a mutation-free context
src/MethodSignatureString.php: [failure] 121-121:
ImpureMethodCall: Cannot call a possibly-mutating method Ray\Aop\TypeString::__invoke from a mutation-free context
Additional comments not posted (6)
src/MethodSignatureString.php (6)
26-29: ConstantsPHP_VERSION_8,NULLABLE_PHP8,NULLABLE_PHP7, andINDENThave been added to manage PHP version checks and formatting. This is a good use of constants to avoid magic numbers and strings throughout the code.
36-36: The constructor now uses a ternary operation to setnullableStrbased on the PHP version. This is a clear and concise way to handle version-dependent features.
42-48: Refactoring of thegetmethod to use helper methods for building the method signature is a good practice as it improves modularity and readability.
51-56: The methodgetDocCommentsimplifies the extraction of the doc comment from a method. Using a ternary operator to check if the comment is a string before appending is a clean approach.
84-89: TheaddAccessModifiersmethod effectively gathers and appends access modifiers to the signature. This method is straightforward and correctly implemented.
119-119: ThegenerateParameterCodemethod effectively constructs parameter strings for the method signature. It handles types, default values, and other parameter attributes correctly.
koriym
force-pushed
the
enum-in-attribute-args
branch
from
b609f43
to
ecee7cf
Compare
The method attribute fetching process has been refactored to improve readability and maintainability. The method attributes are now fetched once and stored in a variable before being iterated over, rather than fetching them directly during the iteration. The 'psalm-suppress MixedAssignment' annotation has been replaced by a 'var mixed' comment, providing easier understanding of the variable's expected value format.
Summary by CodeRabbit
New Features
FakeMarker5andFakeMarker6for method annotations.FakePhp81Enumwith casesApple,Orange, andGrape.Enhancements
testVariousMethodSignatureto include additional assertions for new methods.Bug Fixes
Documentation