create type safety #151
create type safety #151
Conversation
Signed-off-by: Robert Landers <landers.robert@gmail.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughThe changes introduce support for specifying and handling expected result types in orchestration futures. Method signatures in orchestration context classes and interfaces are updated to accept optional result type parameters and variadic arguments. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OrchestrationContext
participant DurableFuture
participant Serializer
Caller->>OrchestrationContext: callActivity(name, returnType, ...args)
OrchestrationContext->>DurableFuture: new DurableFuture(future, returnType)
DurableFuture->>DurableFuture: getResult()
alt returnType is provided
DurableFuture->>Serializer: deserialize(awaitedResult, returnType)
Serializer-->>DurableFuture: deserializedResult
DurableFuture-->>Caller: deserializedResult
else returnType is null
DurableFuture-->>Caller: awaitedResult
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Signed-off-by: Robert Landers <landers.robert@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/OrchestrationContext.php (1)
73-96: Fix missingRetryOptionshandling incallActivityThe
$retryOptionsparameter insrc/OrchestrationContext.php::callActivityis never used—retries aren’t being scheduled. To address this, either:
- Implement retry logic by passing
$retryOptionsinto your task scheduling (e.g. extendScheduleTask::forName()or use a different scheduler method that accepts retry settings), or- Remove the unused
$retryOptionsparameter from both the interface and implementation if retries will not be supported here.Locations to update:
src/OrchestrationContext.php(methodcallActivity)src/OrchestrationContextInterface.php(parameter definition)src/Testing/DummyOrchestrationContext.php(ensure dummy context signature matches)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/DurableFuture.php(1 hunks)src/OrchestrationContext.php(10 hunks)src/OrchestrationContextInterface.php(4 hunks)src/Testing/DummyOrchestrationContext.php(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Testing/DummyOrchestrationContext.php (3)
src/OrchestrationContextInterface.php (1)
waitForExternalEvent(140-140)src/OrchestrationContext.php (1)
waitForExternalEvent(239-255)src/DurableFuture.php (1)
DurableFuture(34-62)
src/OrchestrationContextInterface.php (3)
src/Testing/DummyOrchestrationContext.php (3)
callActivity(107-125)waitForExternalEvent(263-270)waitAll(357-368)src/OrchestrationContext.php (3)
callActivity(73-96)waitForExternalEvent(239-255)waitAll(426-445)src/DurableFuture.php (1)
DurableFuture(34-62)
🪛 PHPMD (2.15.0)
src/OrchestrationContext.php
73-73: Avoid unused parameters such as '$retryOptions'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests - PHP
- GitHub Check: build-docker
🔇 Additional comments (17)
src/Testing/DummyOrchestrationContext.php (4)
150-150: LGTM - Code style consistency improvement.Adding space after the negation operator improves readability and maintains consistency with the formatting applied across the codebase.
263-270: LGTM - Proper implementation of typed result support.The method signature correctly matches the updated interface, and the
$resultTypeparameter is properly passed to theDurableFutureconstructor to enable typed deserialization of external event results. The implementation maintains backward compatibility while supporting the new type safety features.
361-361: LGTM - Code style consistency improvement.Consistent formatting with space after negation operator.
379-379: LGTM - Code style consistency improvement.Consistent formatting with space after negation operator.
src/DurableFuture.php (4)
28-29: LGTM - Necessary imports for new functionality.The
Serializerimport is required for the conditional deserialization feature, and importingLogicExceptionis better practice than using the fully qualified name.
37-40: LGTM - Well-designed constructor enhancement.The optional
$resultTypeparameter maintains backward compatibility while enabling type-safe deserialization. The PHPDoc annotations correctly document the generic type relationship.
48-52: LGTM - Correct conditional deserialization implementation.The logic correctly handles both cases: returning the raw result when no type is specified (maintaining backward compatibility) and deserializing to the specified type when provided. The implementation is clean and maintains the existing contract.
55-55: LGTM - Cleaner exception handling.Using the imported
LogicExceptionclass is better practice than the fully qualified name.src/OrchestrationContext.php (6)
112-137: LGTM - Proper implementation of typed future creation.The
createFuturemethod correctly accepts and propagates the optional$resultTypeparameter to bothDurableFutureconstructor calls, enabling consistent typed deserialization across different execution paths.
79-84: LGTM - Improved code formatting.Converting to arrow functions improves readability and consistency throughout the codebase.
119-119: LGTM - Code style consistency improvements.Adding spaces after negation operators improves readability and maintains consistent formatting across the codebase.
Also applies to: 373-373, 460-460, 580-580
218-223: LGTM - Consistent arrow function formatting.Arrow function conversion improves code consistency and readability.
239-255: LGTM - Proper implementation of typed external event waiting.The method signature correctly implements the interface update, and the
$resultTypeparameter is properly passed to theDurableFutureconstructor to enable typed deserialization of external event data.
450-450: LGTM - Enhanced type documentation.Adding the template annotation improves static analysis and IDE support for the generic type parameter.
src/OrchestrationContextInterface.php (3)
44-49: LGTM - Well-designed interface enhancement for type safety.The method signature changes support type-safe activity results while maintaining backward compatibility. The parameter order (name, returnType, retryOptions, ...args) is logical, and the variadic arguments provide better flexibility than the previous array-based approach.
137-140: LGTM - Consistent interface enhancement for typed external events.The method signature correctly supports typed deserialization of external event results, and both implementations (
OrchestrationContextandDummyOrchestrationContext) properly implement this interface contract.
204-210: LGTM - Accurate generic type documentation.The template annotations correctly document that
waitOneaccepts aDurableFuture<T>and returns the resolved result of typeT.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Testing/DummyOrchestrationContext.php (1)
107-126: Method signature successfully updated for type safety.The changes correctly align with the type safety objectives by adding
$returnTypeparameter and using variadic arguments. TheDurableFutureconstructor call is properly updated to pass the return type.However, the
$retryOptionsparameter is unused in this test implementation.Consider removing the unused parameter or adding a comment explaining why it's kept for interface compliance:
public function callActivity( string $name, ?string $returnType = null, - ?RetryOptions $retryOptions = null, mixed ...$args, ): DurableFuture {Or add a comment:
public function callActivity( string $name, ?string $returnType = null, + ?RetryOptions $retryOptions = null, // Kept for interface compliance, not used in test context - ?RetryOptions $retryOptions = null, mixed ...$args, ): DurableFuture {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Testing/DummyOrchestrationContext.php(6 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Testing/DummyOrchestrationContext.php
110-110: Avoid unused parameters such as '$retryOptions'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (2)
src/Testing/DummyOrchestrationContext.php (2)
264-271: LGTM! Type safety enhancement applied correctly.The method signature update and
DurableFutureconstructor call are consistent with the type safety objectives, enabling typed external event handling.
151-151: Good code style improvement.Adding spaces after the negation operator improves readability and maintains consistent formatting throughout the codebase.
Also applies to: 362-362, 380-380
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #151 +/- ##
============================================
- Coverage 46.35% 46.31% -0.04%
- Complexity 1248 1250 +2
============================================
Files 99 99
Lines 3562 3565 +3
============================================
Hits 1651 1651
- Misses 1911 1914 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/OrchestrationHistoryTest.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Unit/OrchestrationHistoryTest.php (3)
src/OrchestrationContextInterface.php (1)
callActivity(49-49)src/Testing/DummyOrchestrationContext.php (2)
callActivity(107-126)DummyOrchestrationContext(58-412)src/Testing/ActivityMock.php (1)
ActivityMock(29-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Unit Tests - PHP
- GitHub Check: build-docker
Signed-off-by: Robert Landers <landers.robert@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/RemoteOrchestrationClient.php (1)
99-103: Enhanced error handling improves debugging capabilities.The try-catch wrapper around
json_decodeprovides better error context by including the raw response body in the error message and preserving the original JsonException. This aligns well with the PR's type safety objectives.Consider these minor improvements:
- } catch (JsonException $exception) { - throw new RuntimeException('Failed to decode JSON: ' . $body, previous: $exception); + } catch (JsonException $exception) { + throw new RuntimeException('Failed to decode JSON response: ' . $body, previous: $exception);Note: Be aware that including the raw response body in error messages might expose sensitive information in logs. Consider truncating or sanitizing the body if it could contain sensitive data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RemoteOrchestrationClient.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (1)
src/RemoteOrchestrationClient.php (1)
38-38: LGTM! Proper exception imports added.The JsonException and RuntimeException imports are correctly added and positioned alphabetically within the existing import block. These are necessary for the enhanced error handling in the
getStatusmethod.Also applies to: 40-40
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Performance Metrics
Performance Metrics
Performance Metrics
|
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Signed-off-by: Robert Landers <landers.robert@gmail.com>
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores