Add NSCopying support to ART*ChannelOptions
#2018
Conversation
Will use in an upcoming commit, but it's also handy for users of the SDK (e.g. I had already found myself wanting it in Chat, when wishing to create a modified version of an options object recevied as an argument).
WalkthroughThe pull request introduces new functionality for handling connection and attachment retry states in the Ably library. It adds Changes
Sequence DiagramsequenceDiagram
participant Original as Original ChannelOptions
participant Copy as Copied ChannelOptions
Original->>Copy: copyWithZone:
Note over Original,Copy: Create independent copy
Original->>Copy: Copy properties
Note over Copy: Ensure unfrozen state
Copy-->>Original: Return new instance
Possibly related PRs
Suggested reviewers
Poem
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
🧹 Nitpick comments (1)
Test/Tests/ChannelOptionsTests.swift (1)
30-47: Consider adding test for params dictionary mutation behavior.While the current test coverage is good, consider adding a test that verifies the behavior when the original params dictionary is modified after copying. This would help document the expected behavior and catch any issues with shallow vs deep copying.
func test_copyRealtimeChannelOptions_paramsModification() throws { let options = ARTRealtimeChannelOptions() options.params = ["foo": "bar"] let copied = try XCTUnwrap(options.copy() as? ARTRealtimeChannelOptions) // Modify original params options.params = ["foo": "baz"] // Verify copied params remain unchanged XCTAssertEqual(copied.params as? [String: String], ["foo": "bar"]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Ably.xcodeproj/project.pbxproj(6 hunks)Source/ARTChannelOptions.m(1 hunks)Source/ARTRealtimeChannelOptions.m(1 hunks)Source/include/Ably/ARTChannelOptions.h(1 hunks)Test/Tests/ChannelOptionsTests.swift(1 hunks)
🧰 Additional context used
📓 Learnings (1)
Source/include/Ably/ARTChannelOptions.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS17_2)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS17_2)
- GitHub Check: build
🔇 Additional comments (7)
Source/include/Ably/ARTChannelOptions.h (1)
11-11: LGTM! Clean addition of NSCopying protocol.The addition of NSCopying protocol is appropriate and aligns with the PR objective.
Source/ARTChannelOptions.m (1)
20-29: LGTM! Well-implemented NSCopying support.The implementation correctly:
- Creates a new instance using alloc/init
- Copies the cipher property
- Explicitly resets the frozen state
- Includes a clear comment explaining the frozen state behavior
Source/ARTRealtimeChannelOptions.m (2)
24-32: LGTM! Well-implemented NSCopying support with proper super call.The implementation correctly:
- Calls super's copyWithZone:
- Copies all instance variables
- Uses consistent direct ivar access
27-27: Consider implications of shallow copying params dictionary.The params dictionary is copied by reference. If it's mutable, changes to the original's params will affect the copy's params. Consider if deep copying is needed for complete isolation.
Let's verify the mutability of params dictionary:
Test/Tests/ChannelOptionsTests.swift (1)
7-18: LGTM! Well-structured test for ARTChannelOptions copying.Test thoroughly verifies:
- Successful copying
- Object identity
- Property copying
Ably.xcodeproj/project.pbxproj (2)
100-102: Project configuration changes look good!The Xcode project has been properly configured with the new test file
ChannelOptionsTests.swiftand its supporting files. The build settings are correctly set up across all necessary targets.Also applies to: 1159-1159, 1738-1738, 3054-3054, 3253-3253, 3312-3312
100-102: Verify test coverage for NSCopying implementation.Let's ensure the new test file covers all necessary scenarios for NSCopying implementation:
- Basic copying behavior
- Deep vs shallow copy behavior
- Immutability preservation
- Property retention accuracy
✅ Verification successful
NSCopying implementation is well-tested ✅
The test coverage is comprehensive, including object identity verification, property retention, frozen state handling, and both
ARTChannelOptionsandARTRealtimeChannelOptionsclasses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for NSCopying implementation # Look for NSCopying protocol conformance echo "Checking NSCopying protocol conformance..." rg -A 5 "NSCopying" Test/ # Look for copy tests echo "Checking copy-related test cases..." rg -A 10 "func test.*[Cc]opy" "Test/Tests/ChannelOptionsTests.swift"Length of output: 2310
Will use in an upcoming commit, but it's also handy for users of the SDK (e.g. I had already found myself wanting it in Chat, when wishing to create a modified version of an options object received as an argument).
Summary by CodeRabbit
New Features
copyWithZone:methodNSCopyingprotocol forARTChannelOptionsTests
Improvements