Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe changes improve error handling and concurrency safety across the codebase. Force-unwraps and force-casts are replaced with safer error-throwing patterns. A public protocol gains a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors Swift code to follow additional best practices, focusing on improving safety and concurrency handling. The changes replace unsafe Swift patterns (force unwrapping, force casting, force try) with safer alternatives, add concurrency annotations, and improve code clarity.
Changes:
- Replaced force unwraps, force casts, and force try operators with safe alternatives using guard statements and proper error propagation
- Added
@Sendableannotations to closure types for Swift concurrency safety - Added Sendable conformance to the Decoder protocol for thread-safe usage
- Removed redundant code in container inspection logic
- Added explanatory comments to defer blocks in tests explaining error suppression rationale
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/TestcontainersTests.swift | Added explanatory comments to defer blocks clarifying why cleanup errors are intentionally suppressed |
| Sources/Testcontainers/WaitStrategy.swift | Added @Sendable annotation to CustomWaitStrategy closure type for concurrency safety |
| Sources/Testcontainers/DockerClient.swift | Replaced force unwrap with guard statement in singleton getInstance; removed redundant container lookup code |
| Sources/DockerClientSwift/Helper/HTTPClient+Codable.swift | Replaced force casts with safe guard-let patterns; added Sendable conformance to Decoder protocol |
| Sources/DockerClientSwift/APIs/DockerClient.swift | Replaced try! with proper try for error propagation in endpoint body encoding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if T.self == NoBody.self || T.self == NoBody?.self { | ||
| return NoBody() as! T | ||
| guard let result = NoBody() as? T else { | ||
| throw BodyError.noBodyData |
There was a problem hiding this comment.
The error thrown here (BodyError.noBodyData) is misleading. This guard is protecting against a type casting failure (NoBody() as? T), not missing body data. The cast should theoretically always succeed when T.self == NoBody.self, so this represents an unexpected type system issue rather than missing data. Consider using a more appropriate error or adding a comment explaining why this cast could fail. If this truly should never fail, a fatalError with a clear message might be more appropriate than throwing noBodyData.
| throw BodyError.noBodyData | |
| fatalError("Internal error: Failed to cast NoBody to expected type \(T.self). This indicates an unexpected type system issue and should never occur.") |
| guard let result = String(data: bodyData, encoding: .utf8) as? T else { | ||
| throw BodyError.noBodyData | ||
| } | ||
| return result |
There was a problem hiding this comment.
Similar to the NoBody case above, the error thrown here (BodyError.noBodyData) is misleading. The String(data:encoding:) call already succeeded (we have bodyData from line 67-68), so this guard is protecting against a type casting failure (String as? T), not missing data or encoding issues. When T.self == String.self, this cast should always succeed. Consider using a more appropriate error message or documenting why this cast could theoretically fail.
| guard let result = String(data: bodyData, encoding: .utf8) as? T else { | |
| throw BodyError.noBodyData | |
| } | |
| return result | |
| guard let string = String(data: bodyData, encoding: .utf8) else { | |
| throw BodyError.noBodyData | |
| } | |
| return string as! T |
| guard let result = NoBody() as? T else { | ||
| throw EventLoopFuture<HTTPClient.Response>.BodyError.noBodyData | ||
| } | ||
| return result |
There was a problem hiding this comment.
Same issue as in the EventLoopFuture variant: BodyError.noBodyData is a misleading error for a type casting failure. When T.self == NoBody.self, the cast NoBody() as? T should always succeed, so this guard represents an unexpected type system issue rather than missing body data.
| guard let result = NoBody() as? T else { | |
| throw EventLoopFuture<HTTPClient.Response>.BodyError.noBodyData | |
| } | |
| return result | |
| // For `NoBody` (and `NoBody?`) the cast is guaranteed by the type check above. | |
| return NoBody() as! T |
| guard let result = String(data: bodyData, encoding: .utf8) as? T else { | ||
| throw EventLoopFuture<HTTPClient.Response>.BodyError.noBodyData | ||
| } | ||
| return result |
There was a problem hiding this comment.
Same issue as above: when T.self == String.self and String(data:encoding:) succeeded (creating a non-nil String), the cast String as? T should always succeed. The BodyError.noBodyData error message is misleading for what is actually a type casting failure scenario.
| guard let result = String(data: bodyData, encoding: .utf8) as? T else { | |
| throw EventLoopFuture<HTTPClient.Response>.BodyError.noBodyData | |
| } | |
| return result | |
| guard let string = String(data: bodyData, encoding: .utf8) else { | |
| throw EventLoopFuture<HTTPClient.Response>.BodyError.noBodyData | |
| } | |
| return string as! T |
| guard let instance = _shared.value else { | ||
| // Should never happen; assignment always succeeds. | ||
| return TestcontainersDockerClient() | ||
| } |
There was a problem hiding this comment.
The fallback logic here creates a new instance instead of returning the shared singleton, defeating the purpose of the singleton pattern. If assignment succeeds (which it should), the guard will never fail. Consider simplifying this to directly return the value after the nil check and assignment, or use a fatalError if this truly should never happen. A safer approach would be:
if _shared.value == nil {
_shared.value = TestcontainersDockerClient()
}
return _shared.value! // Safe: we just assigned above if nilOr with a more explicit check:
_shared.value = _shared.value ?? TestcontainersDockerClient()
return _shared.value!The fallback to creating a new instance silently creates non-singleton behavior without any indication that something went wrong.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (48.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
- Coverage 62.36% 61.96% -0.41%
==========================================
Files 40 40
Lines 1201 1212 +11
==========================================
+ Hits 749 751 +2
- Misses 452 461 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
Description
Implemented swift additional best practices
Type of Change
Continue Tasks: ❌ 3 failed — View all
Summary by CodeRabbit
Bug Fixes
Improvements
Tests