Rename Subscription’s finish method to unsubscribe
#168
Conversation
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve renaming the Changes
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
🧹 Outside diff range and nitpick comments (2)
Sources/AblyChat/Subscription.swift (2)
Line range hint
75-82: Update error message to match the new method name.The rename from
finishtounsubscribeimproves API clarity by aligning with JavaScript conventions. However, the error message still references the old method name.Apply this diff to update the error message:
public func unsubscribe() { switch mode { case let .default(_, continuation): continuation.finish() case .mockAsyncSequence: - fatalError("`finish` cannot be called on a Subscription that was created using init(mockAsyncSequence:)") + fatalError("`unsubscribe` cannot be called on a Subscription that was created using init(mockAsyncSequence:)") } }
73-73: Consider documenting the temporary nature of this method.The TODO comment and PR context indicate that this unsubscription mechanism might be revised. Consider adding a note in the documentation that this API might change before the final release.
Would you like me to suggest documentation text that clarifies the potential future changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Sources/AblyChat/Subscription.swift(1 hunks)
🔇 Additional comments (1)
Sources/AblyChat/Subscription.swift (1)
Line range hint 75-82: Verify the impact of the rename across the codebase.
Let's ensure all callers of the previous finish() method have been updated.
✅ Verification successful
No impact found from the rename of finish() to unsubscribe()
The method finish() was only used internally within the Subscription class to handle the continuation. The rename to unsubscribe() is a public API change that doesn't affect any existing code as:
- The only usage of
finish()is internal to theSubscriptionclass itself - No direct calls to either
finish()orunsubscribe()were found in the codebase - The
Subscriptionclass is used throughout the codebase but primarily for emitting events and subscribing to changes, not for cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old method name
rg -i "\.finish\(\)" --type swift
# Search for the new method name to verify usage
rg -i "\.unsubscribe\(\)" --type swift
Length of output: 146
Script:
#!/bin/bash
# Let's search for any test files or usage of Subscription class
rg -l "Subscription" --type swift
# Let's also check for any potential usages of the class pattern
ast-grep --pattern 'let $_ = Subscription'
ast-grep --pattern 'var $_ = Subscription'
ast-grep --pattern 'let $_ : Subscription'
ast-grep --pattern 'var $_ : Subscription'
Length of output: 1945
Script:
#!/bin/bash
# Let's check the test files and implementation files for any direct usage of finish()
rg -A 5 -B 5 "Subscription" Tests/AblyChatTests/SubscriptionTests.swift
rg -A 5 -B 5 "Subscription" Tests/AblyChatTests/MessageSubscriptionTests.swift
rg -A 5 -B 5 "Subscription" Sources/AblyChat/RoomLifecycleManager.swift
# Let's also check for any protocol conformance or inheritance
ast-grep --pattern 'class $_ : Subscription'
ast-grep --pattern 'class Subscription'
Length of output: 14651
|
Tests are failing due to #169. |
I don’t think that this method should exist at all; the intention for our public API, as described in 20e7f5f, is that the user should not need to explicitly unsubscribe. (We have #36 for figuring out how to implement the appropriate cleanup.) However, this method does now exist, and we’re probably not going to get rid of it before beta, so we agreed in standup today to at least name it in line with JS.
lawrence-forooghian
force-pushed
the
rename-finish-to-unsubscribe
branch
from
300f5f8 to
501a1d5
Compare
I don’t think that this method should exist at all; the intention for our public API, as described in 20e7f5f, is that the user should not need to explicitly unsubscribe. (We have #36 for figuring out how to implement the appropriate cleanup.) However, this method does now exist, and we’re probably not going to get rid of it before beta, so we agreed in standup today to at least name it in line with JS.
Summary by CodeRabbit
New Features
finish()method tounsubscribe()for better clarity in subscription management.Bug Fixes
unsubscribe()method retains the same functionality as the previousfinish()method.Documentation