[ECO-5167] Make bufferingPolicy parameter optional in public API
#182
Conversation
`.unbounded` is the default value.
Not sure why I didn’t give these APIs such a parameter when I introduced the public API in 20e7f5f; if there was a good reason then it’s not obvious to me now.
When I introduced the public API in 20e7f5f, I didn’t want users to be in a situation where there was an buffer of unbounded size growing without them even knowing about it, and I hence forced them to be explicit about their choice of buffering policy. But, a couple of things: - Marat pointed out that this makes our code examples and README look quite cluttered, with `bufferingPolicy` arguments all over the place. This distracts from demonstrating the functionality of the library. - Swift’s own `AsyncStream` defaults to a buffering policy of `.unbounded`. If doing so is good enough for them, then it’s probably good enough for us. So, default to `.unbounded`. Resolves #172.
WalkthroughThe pull request introduces significant modifications to subscription methods across various components of the SwiftUI chat application, particularly focusing on the Changes
Assessment against linked issues
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: 4
🧹 Outside diff range and nitpick comments (4)
Sources/AblyChat/RoomReactions.swift (1)
7-11: LGTM! Consider enhancing the documentation.The new protocol method addition perfectly aligns with the PR objective of making
bufferingPolicyoptional. The documentation clearly explains the relationship with the original method.Consider adding a code example to the documentation to show the simplified usage:
/// Same as calling ``subscribe(bufferingPolicy:)`` with ``BufferingPolicy.unbounded``. /// /// The `RoomReactions` protocol provides a default implementation of this method. +/// +/// Example usage: +/// ```swift +/// let subscription = await reactions.subscribe() +/// ``` func subscribe() async -> Subscription<Reaction>Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
Line range hint
52-58: Consider makingsubscribeToState()consistent with the new pattern.While outside the immediate scope of this PR, the
subscribeToState()method still uses a hardcoded.unboundedbuffering policy. Consider making it consistent with the new pattern by:
- Adding a parameter for bufferingPolicy
- Providing a convenience method with default
.unbounded-internal func subscribeToState() async -> Subscription<ARTChannelStateChange> { +internal func subscribeToState(bufferingPolicy: BufferingPolicy = .unbounded) async -> Subscription<ARTChannelStateChange> { // TODO: clean up old subscriptions (https://github.com/ably-labs/ably-chat-swift/issues/36) - let subscription = Subscription<ARTChannelStateChange>(bufferingPolicy: .unbounded) + let subscription = Subscription<ARTChannelStateChange>(bufferingPolicy: bufferingPolicy) underlyingChannel.on { subscription.emit($0) } return subscription }Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
Line range hint
76-76: Update subscribeToDiscontinuities test to include bufferingPolicy.The test for
subscribeToDiscontinuitiesshould be updated to include thebufferingPolicyparameter to match the implementation changes inDefaultOccupancy.swift.Apply this diff to fix the test:
- let messagesDiscontinuitySubscription = await roomReactions.subscribeToDiscontinuities() + let messagesDiscontinuitySubscription = await roomReactions.subscribeToDiscontinuities(bufferingPolicy: .unbounded)Sources/AblyChat/Messages.swift (1)
5-8: Documentation could be more descriptiveWhile the documentation is accurate, it could be enhanced to:
- Explain why
.unboundedis the default choice- Mention any performance implications
- Add a code example showing usage
Here's a suggested enhancement:
- /// Same as calling ``subscribe(bufferingPolicy:)`` with ``BufferingPolicy.unbounded``. - /// - /// The `Messages` protocol provides a default implementation of this method. + /// Subscribes to messages with an unbounded buffering policy. + /// + /// This method provides a simplified API for common use cases where message buffering + /// is not a concern. It uses `.unbounded` policy which stores all messages in memory + /// until they are consumed. + /// + /// Example usage: + /// ```swift + /// let subscription = try await messages.subscribe() + /// for await message in subscription { + /// // Process message + /// } + /// ``` + /// + /// The `Messages` protocol provides a default implementation of this method. + /// + /// - Returns: A `MessageSubscription` that can be used to receive messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
Example/AblyChatExample/ContentView.swift(6 hunks)Example/AblyChatExample/Mocks/MockClients.swift(5 hunks)Example/AblyChatExample/Mocks/MockSubscription.swift(1 hunks)Sources/AblyChat/Connection.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(3 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(1 hunks)Sources/AblyChat/DefaultRoomReactions.swift(1 hunks)Sources/AblyChat/DefaultTyping.swift(2 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(1 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(1 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)
🔇 Additional comments (33)
Sources/AblyChat/Presence.swift (3)
85-88: LGTM: Addition of Default 'subscribe' Methods Enhances Usability
The inclusion of the overloaded subscribe methods without the bufferingPolicy parameter simplifies the API and improves usability by providing default behavior.
90-93: LGTM: Overloaded Methods for Multiple Events Improve API Flexibility
Adding subscribe methods that accept an array of PresenceEventType without requiring a bufferingPolicy enhances the flexibility and convenience of the API.
97-105: LGTM: Default Implementations Correctly Delegate to Parameterized Methods
The default implementations in the Presence extension correctly delegate to the parameterized methods using .unbounded buffering policy, ensuring consistent behavior.
Sources/AblyChat/Typing.swift (2)
5-8: LGTM: Addition of Default 'subscribe' Method Simplifies API
Introducing the subscribe() method without a bufferingPolicy parameter enhances usability by allowing users to subscribe with default settings.
15-19: LGTM: Default Implementation Ensures Consistent Behavior
The default implementation of subscribe() in the Typing extension correctly forwards the call to subscribe(bufferingPolicy: .unbounded), maintaining consistent behavior across the API.
Sources/AblyChat/EmitsDiscontinuities.swift (1)
20-24: LGTM: Default Implementation Appropriately Delegates Calls
The extension's subscribeToDiscontinuities() method correctly delegates to subscribeToDiscontinuities(bufferingPolicy: .unbounded), ensuring default behavior is applied consistently.
Sources/AblyChat/Occupancy.swift (2)
5-8: LGTM: Added Default 'subscribe' Method Enhances API Convenience
The new subscribe() method without a bufferingPolicy parameter simplifies the subscription process, making the API more user-friendly.
13-17: LGTM: Extension Provides Correct Default Implementation
The default implementation in the Occupancy extension appropriately calls subscribe(bufferingPolicy: .unbounded), ensuring consistent default behavior.
Sources/AblyChat/RoomReactions.swift (1)
13-16: LGTM! Clean implementation of the default behavior.
The extension provides a straightforward implementation that maintains the expected behavior while simplifying the API.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
18-22: LGTM! Implementation correctly mirrors the real component.
The mock implementation properly accepts and uses the bufferingPolicy parameter. Note that the subscription cleanup TODO is tracked in issue #36 and is unrelated to this PR's scope.
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
Line range hint 21-26: LGTM! Implementation correctly uses the provided bufferingPolicy.
The changes align with the PR objective and maintain consistency with other components.
Sources/AblyChat/Connection.swift (2)
8-12: LGTM! Well-documented protocol addition.
The new protocol method is well-documented, clearly indicating its relationship to the original method and default implementation.
14-17: LGTM! Clean default implementation.
The extension provides a clean implementation that correctly forwards to the original method with .unbounded buffering policy, aligning with the PR objectives.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
58-58: LGTM! Test updated to use new API.
Test correctly uses the new subscribe method without bufferingPolicy parameter, aligning with the PR objectives.
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
20-20: LGTM! Test cases updated to use simplified API
The test cases have been appropriately updated to use the new subscribe() method without the bufferingPolicy parameter. This change aligns with making the parameter optional and defaulting to .unbounded.
Also applies to: 59-59
Sources/AblyChat/Messages.swift (1)
14-18: LGTM! Clean implementation of default subscription
The extension provides a clean implementation of the simplified API while maintaining full functionality through the original method. This is a good solution for providing default parameter values in Swift protocols.
Sources/AblyChat/DefaultRoomReactions.swift (1)
83-84: LGTM: Method signature and implementation are correct.
The change properly forwards the bufferingPolicy parameter to the feature channel's implementation.
Sources/AblyChat/DefaultTyping.swift (2)
25-25: LGTM: Subscription creation with buffering policy is correct.
The implementation properly initializes the subscription with the provided buffering policy while maintaining the existing robust error handling and retry mechanism.
163-164: LGTM: Method signature and implementation are correct.
The change properly forwards the bufferingPolicy parameter to the feature channel's implementation.
Example/AblyChatExample/Mocks/MockClients.swift (1)
147-147: LGTM: Mock implementations consistently updated.
All mock implementations have been consistently updated to include the bufferingPolicy parameter, maintaining API compatibility. The use of fatalError("Not yet implemented") is appropriate for mock objects in example/testing code.
Also applies to: 198-198, 247-247, 343-343, 347-347, 351-351, 386-386
Sources/AblyChat/Room.swift (2)
17-20: LGTM: Well-documented protocol method addition
The new method provides a simpler API by defaulting to .unbounded buffering policy, which aligns with the PR objectives.
26-30: LGTM: Clean default implementation
The protocol extension provides a clean default implementation that maintains backward compatibility while simplifying the API.
Sources/AblyChat/DefaultPresence.swift (3)
Line range hint 167-177: LGTM: Proper integration of bufferingPolicy parameter
The method correctly integrates the bufferingPolicy parameter while maintaining existing logging and error handling.
Line range hint 181-193: LGTM: Consistent implementation for multi-event subscription
The implementation maintains consistency with the single-event subscription method and properly handles the bufferingPolicy parameter.
197-199: LGTM: Proper forwarding of bufferingPolicy
The method correctly forwards the bufferingPolicy parameter to the feature channel's discontinuities subscription.
Sources/AblyChat/DefaultMessages.swift (1)
112-114: LGTM: Consistent implementation of bufferingPolicy parameter
The method maintains consistency with other components by properly handling the bufferingPolicy parameter.
Example/AblyChatExample/ContentView.swift (5)
172-172: LGTM: Simplified subscription call by removing explicit bufferingPolicy.
The change aligns with making the bufferingPolicy parameter optional in the public API, defaulting to .unbounded.
192-192: LGTM: Simplified reactions subscription.
Removed explicit bufferingPolicy parameter, utilizing the default .unbounded value.
222-222: LGTM: Simplified typing subscription.
Removed explicit bufferingPolicy parameter, utilizing the default .unbounded value.
244-244: LGTM: Simplified occupancy subscription.
Removed explicit bufferingPolicy parameter, utilizing the default .unbounded value.
253-253: LGTM: Simplified status change subscriptions.
Both onStatusChange() calls now use the default .unbounded buffering policy, making the API more concise.
Also applies to: 266-266
Tests/AblyChatTests/IntegrationTests.swift (1)
87-87: LGTM: Updated test cases to use simplified subscription API.
Test cases have been properly updated to reflect the API changes where bufferingPolicy parameter is now optional. The changes maintain test coverage while using the new simplified API.
Also applies to: 101-101, 111-111, 155-155, 173-173, 246-246
Tests/AblyChatTests/DefaultRoomTests.swift (1)
287-287: LGTM: Updated test to use simplified onStatusChange API.
Test case properly updated to use the new method signature without the bufferingPolicy parameter.
lawrence-forooghian
deleted the
172-subscribe-without-buffering-policy
branch
Use a default buffering policy of
.unbounded. Also, I’ve added some missingbufferingPolicyparameters that for some reason I forgot to include in 20e7f5f. See commit messages for more details.Resolves #172.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor