Add dependencyIndex to the message of the Unbound exception #113
Conversation
|
この変更によるエラー失敗ではないですが、エラーでCIが落ちてますね。元々のテストの書き方の問題でもあるようですが... |
|
そうなんです。php-parserのphp>=7.1対応の4系と、php>=7.4対応の5系で出力されるコードが異なってしまっているようで、どうするのが良さそうでしょうか？ |
|
arrayや鉤括弧を確認する必要もないのでその確認の詳細度合いが過ぎてるところはもう削除してしまっていいと思います。 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 1.x #113 +/- ##
=============================================
+ Coverage 99.86% 100.00% +0.13%
Complexity 215 215
=============================================
Files 28 28
Lines 740 739 -1
=============================================
Hits 739 739
+ Misses 1 0 -1 ☔ View full report in Codecov by Sentry. |
|
@koriym lowerstとhighestで、バックスラッシュを |
WalkthroughThe changes in this pull request involve modifications to three files: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CompileInjector
participant DependencyCode
participant DependencyCompilerTest
User->>CompileInjector: Request to compile file
CompileInjector->>CompileInjector: Check if file exists
alt File does not exist
CompileInjector-->>User: Throw Unbound exception with $dependencyIndex
else File exists
CompileInjector-->>User: Return compiled file
end
User->>DependencyCode: Request provider code
DependencyCode->>DependencyCode: Suppress InvalidArgument warnings
DependencyCode-->>User: Return provider code
User->>DependencyCompilerTest: Run tests
DependencyCompilerTest->>DependencyCompilerTest: Execute assertions
DependencyCompilerTest-->>User: Return test results
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 (
|
|
@jingu GJ！ |
![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 (4)
tests/DependencyCompilerTest.php (2)
83-90: Improved test flexibility with complex string handling.The change from
assertSametoassertContainswith multiple possible outcomes is appropriate. It addresses the discrepancies in code representation between different PHP versions, aligning with the PR objectives. The complex string replacement handles both array syntax differences and backslash escaping issues effectively.However, the readability of this section could be improved. Consider extracting the string replacement logic into a separate helper method to enhance clarity.
Here's a suggested refactoring to improve readability:
private function getExpectedStrings(string $expected): array { $modernSyntax = str_replace( 'array(\'Ray\\Compiler\\FakeCar\', \'setHandle\', \'handle\')', '[\'Ray\\Compiler\\FakeCar\', \'setHandle\', \'handle\']', str_replace('\\\\', '\\', $expected) ); return [$modernSyntax, $expected]; } // In the test method $this->assertContains((string) $code, $this->getExpectedStrings($expected));
Line range hint
1-168: Overall assessment: Effective improvements in test flexibilityThe changes in this file consistently improve test flexibility to accommodate differences between PHP versions, particularly in array syntax and serialized object representation. This aligns well with the PR objectives and addresses the issues discussed in the PR comments.
To further enhance the maintainability of these tests, consider:
- Extracting the string replacement logic into helper methods, as suggested earlier.
- Adding comments explaining why these flexible assertions are necessary, referencing the PHP version discrepancies.
- If this pattern is used in multiple test files, consider creating a trait with these helper methods to promote code reuse.
These suggestions will improve code organization, readability, and make it easier for future contributors to understand and maintain these tests.
src/DependencyCode.php (1)
Remaining occurrences of 'setQaulifier' found.
Please update the following locations to correct the typo:
src/DependencyCode.phpsrc/FunctionCode.php🔗 Analysis chain
Line range hint
79-82: Approve typo correction and suggest verification.The typo correction in the method name and parameter improves code consistency and readability. Good catch!
To ensure this change doesn't break existing code, please verify all calls to this method have been updated. Run the following script to check for any remaining occurrences of the old method name:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining occurrences of 'setQaulifier' rg 'setQaulifier' --type phpLength of output: 219
src/CompileInjector.php (1)
171-171: Improved error message with dependency context.The change enhances the error message by including the
$dependencyIndexand referencing the compile log. This aligns with the PR objective and improves error reporting.Consider using a constant for the log file name (
'_compile.log') to improve maintainability:- throw new Unbound(sprintf('[%s] See compile log %s', $dependencyIndex, $this->scriptDir . '/_compile.log')); + const COMPILE_LOG = '_compile.log'; + throw new Unbound(sprintf('[%s] See compile log %s', $dependencyIndex, $this->scriptDir . '/' . self::COMPILE_LOG));This change would make it easier to update the log file name in the future if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
- src/CompileInjector.php (1 hunks)
- src/DependencyCode.php (1 hunks)
- tests/DependencyCompilerTest.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/DependencyCompilerTest.php (2)
56-59: Improved test flexibility for array representation.The change from
assertSametoassertContainswith multiple possible outcomes is a good approach. It addresses the discrepancies in array representation between different PHP versions (7.1 vs 7.4) mentioned in the PR discussion. This modification enhances the test's robustness and aligns with the PR objectives.
133-136: Improved test flexibility for serialized object representation.The change from
assertSametoassertContainswith multiple possible outcomes is a good approach. It addresses potential discrepancies in serialized object representation between different PHP versions. This modification enhances the test's robustness and aligns with the PR objectives.src/DependencyCode.php (1)
153-153: Clarify the need for@psalm-suppress InvalidArgument.The addition of this suppression annotation suggests that Psalm is detecting a potential type mismatch. While suppressing the warning allows the code to pass static analysis, it might hide an underlying issue.
Could you please provide more context on why this suppression is necessary? Are there alternatives we could explore to address the underlying issue instead of suppressing the warning?
To help understand the context, please run the following command to see Psalm's output for this file:
This will help us understand the specific warning that Psalm is raising and potentially find a way to resolve it without suppression.
src/CompileInjector.php (1)
Line range hint
1-214: Overall impact: Improved error reporting without side effects.The change to the error message in
getInstanceFilemethod enhances the debugging experience by providing more context about the unbound dependency. This modification:
- Aligns with the PR objectives.
- Doesn't alter the method's logic or return value.
- Improves error reporting without introducing new dependencies or failure modes.
- Is consistent with the class's responsibility of managing compiled scripts.
No additional changes are required in other parts of the class or file.
Summary by CodeRabbit
Bug Fixes
setQaulifiertosetQualifier, ensuring consistent naming.Tests