-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use Cancellation object instead of unsubscribe methods #15
Conversation
WalkthroughThe changes introduced in this pull request primarily involve modifications to several interfaces within the chat application, focusing on listener management and cancellation mechanisms. The existing methods for unregistering listeners have been removed, and corresponding methods have been updated to return a Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range and nitpick comments (2)
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
114-116
: Approve the method signature change. Reminder to implement the method.The method signature change to return a
Cancellation
object is necessary to align with the interface change and is correct.The missing implementation is acceptable at this stage, as indicated by the
TODO
comment. Please ensure to implement the method in a future PR.
118-120
: Approve the method signature change. Reminder to implement the method.The change to return a
Cancellation
object from theonDiscontinuity
method is consistent with the changes made to thesubscribe
method and indicates that it now similarly supports cancellation functionality. This change is a good improvement.The missing implementation is acceptable at this stage, as indicated by the
TODO
comment. Please ensure to implement the method in a future PR.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .gitignore (1 hunks)
- chat-android/src/main/java/com/ably/chat/Cancellation.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Messages.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Occupancy.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Presence.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Typing.kt (3 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments not posted (12)
chat-android/src/main/java/com/ably/chat/Cancellation.kt (1)
1-12
: Excellent addition of theCancellation
interface!The introduction of the
Cancellation
interface is a great step towards refactoring the subscription pattern to use a cancellation object approach. This change brings several benefits to the codebase:
Consistency: The interface promotes a consistent approach to handling cancellations across the codebase, making it easier for developers to understand and work with the subscription mechanism.
Simplicity: By consolidating the subscription and unsubscription processes into a single interface, the API becomes simpler and more intuitive to use.
Clarity: The interface has a clear and focused responsibility of providing a cancellation mechanism, which is well-documented through the concise comments.
Future-proofing: The interface lays the foundation for potential future enhancements, such as the introduction of a
use
method in Kotlin, which could facilitate cleaner syntax and improved resource management.Overall, the
Cancellation
interface is a valuable addition to the codebase, aligning with the PR objectives and setting the stage for a more streamlined and developer-friendly subscription pattern.chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1)
13-13
: LGTM! Verify the usage of theCancellation
object in the codebase.The change to return a
Cancellation
object aligns with the PR objective to refactor the subscription pattern to use a cancellation object approach. This change simplifies the API by consolidating the subscription and unsubscription processes into a single interface. The removal of theoffDiscontinuity
function is consistent with the new approach.These changes enhance consistency across the codebase and set the stage for future enhancements, such as the potential introduction of a
use
method in Kotlin.Run the following script to verify the usage of the
Cancellation
object:Verification successful
Verified:
Cancellation
object is consistently used foronDiscontinuity
and similar methodsThe
onDiscontinuity
function now correctly returns aCancellation
object across multiple classes (RoomReactions, Presence, Typing, Occupancy, and Messages). This pattern is consistently applied not only toonDiscontinuity
but also to other subscription-like methods such assubscribe
andon
.The
Cancellation
interface is properly defined with acancel()
method for handling unsubscription and cleanup, aligning with the PR objective to refactor the subscription pattern.Note: While the structure is in place, most implementations currently use
TODO("Not yet implemented")
. This suggests that the actual logic for these methods still needs to be implemented in a future task.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Cancellation` object returned by `onDiscontinuity`. # Test 1: Search for the `onDiscontinuity` function calls. # Expect: The return value should be used to unsubscribe. rg --type kotlin -A 5 $'onDiscontinuity' # Test 2: Search for the `Cancellation` type usage. # Expect: Should be used to store the return value of `onDiscontinuity`. rg --type kotlin -A 5 $'Cancellation'Length of output: 10412
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
26-26
: LGTM!The modification to the
subscribe
method to return aCancellation
object aligns with the PR objective of refactoring the subscription pattern. This change simplifies the API by consolidating the subscription and unsubscription processes into a single interface, providing better control over occupancy updates.chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt (1)
28-28
: Excellent refactor to simplify the listener management!The change to the
on
method signature, returning aCancellation
object, is a great improvement. It consolidates the listener registration and cancellation into a single method, making the interface more intuitive and easier to use.By removing the
off
method and relying on theCancellation
object for listener management, the code becomes cleaner and less error-prone. This approach reduces the chances of memory leaks or dangling references that could occur when listeners are not properly unregistered.Overall, this refactor enhances the design of the
ConnectionStatus
interface, making it more robust and aligned with modern practices.chat-android/src/main/java/com/ably/chat/RoomStatus.kt (1)
24-24
: LGTM!The change to the
on
method signature, returning aCancellation
object, simplifies the listener management and aligns with the PR objective of refactoring the subscription pattern. This change improves the clarity and usability of the interface.chat-android/src/main/java/com/ably/chat/Typing.kt (1)
40-40
: LGTM!The change to the
subscribe
method signature aligns with the PR objective to refactor the subscription pattern to return a cancellation object. This simplifies the API by consolidating the subscription and unsubscription processes into a single interface.chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
41-41
: LGTM!The change to return a
Cancellation
object from thesubscribe
method is a good improvement. It provides a mechanism to cancel the subscription and enhances control over listener management. This change simplifies the API by consolidating the subscription and unsubscription processes into a single interface, which aligns with the PR objectives and contemporary practices in library design.chat-android/src/main/java/com/ably/chat/Presence.kt (2)
63-63
: LGTM!The change in the
subscribe
function signature to return aCancellation
object aligns well with the PR objective of using a cancellation object for unsubscribing, instead of a separateunsubscribe
method. This simplifies the API by consolidating the subscription and unsubscription processes into a single interface.The corresponding removal of the
unsubscribe
function is also consistent with the new subscription model using theCancellation
object.
163-163
: LGTM!The change in the
onDiscontinuity
function to return aCancellation
object aligns well with the new subscription model introduced in thesubscribe
function. This ensures consistency across the codebase.Also, the renaming of the function from
offDiscontinuity
toonDiscontinuity
improves clarity and maintains consistency with the naming convention used for other event listener functions.chat-android/src/main/java/com/ably/chat/Messages.kt (3)
26-26
: LGTM!The change to return a
MessagesSubscription
object from thesubscribe
method is a good improvement. It simplifies the subscription mechanism by consolidating the subscription and unsubscription processes into a single interface, aligning with the PR objectives.
172-174
: Looks good!The new
MessagesSubscription
interface aligns well with the refactored subscription model. ExtendingCancellation
allows for better control over the subscription lifecycle. The addition of thegetPreviousMessages
method enhances the functionality by enabling retrieval of historical messages based on query options.
194-194
: No changes required.The
get
method implementation remains unchanged, indicating that the message retrieval functionality is not impacted by the subscription refactor. The existing implementation looks good.
It's proven to be easier to manage to implement when subscription return object that you can use to unsubscribe
14aecaf
to
54ecb93
Compare
/** | ||
* Handle cancellation (unsubscribe listeners, clean up) | ||
*/ | ||
fun cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JS we use unsubscribe
or off
but happy to keep cancel
if that's more kotlin-idiomatic.
@ttypic I am skeptical about use of
While both of those languages are pretty old and natively support passing cancellation tokens, those cancel tokens are only used to manage dotnet tasks / goroutines. In both ably-dotnet and ably-go, we return |
@sacOO7 Not sure what are you suggesting? And I don't see why we have to have 2 different functional interfaces for unsubscription (one with Also I expect that chat sdk users won't use this methods at all, we'll provide extensions for coroutines and maybe some other reactive frameworks. |
Okay, can we use some other keyword instead of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Outside diff range and nitpick comments (9)
chat-android/src/main/java/com/ably/chat/Unsubscribable.kt (2)
3-6
: Refine the interface documentation for clarityConsider rephrasing the class-level documentation to provide a clearer and more detailed description of the
Unsubscribable
interface. This will improve understandability for other developers.You can apply this diff to enhance the documentation:
/** - * An unsubscription handle, returned by various functions (mostly subscriptions) - * where unsubscription is required. + * Represents a handle that allows unsubscribing from subscriptions. + * Implementations define how to remove listeners and clean up resources. */
8-10
: Improve the method documentation for consistencyIt's recommended to start method documentation comments with a third-person singular verb to align with standard documentation conventions.
Apply this diff to adjust the method documentation:
/** - * Handle unsubscription (unsubscribe listeners, clean up) + * Unsubscribes from the subscription, removing listeners and cleaning up resources. */chat-android/src/main/java/com/ably/chat/Typing.kt (1)
40-42
: Update documentation to reflect the new return typeThe
subscribe
method now returns anUnsubscribable
, but the documentation does not mention this return value. Please update the method documentation to explain the purpose of the returnedUnsubscribable
and how it should be used to unsubscribe.chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
41-41
: Consider renamingUnsubscribable
toSubscription
for clarityThe term
Unsubscribable
might be less intuitive for users. UsingSubscription
aligns with common conventions and clearly indicates the purpose of the returned object.chat-android/src/main/java/com/ably/chat/Presence.kt (3)
159-161
: Implement thesubscribe
method to return a functional cancellation objectThe
subscribe
method is currently not implemented. Ensure that the returnedUnsubscribable
object correctly handles unsubscription to prevent memory leaks and unintended behavior.Would you like assistance in implementing this method?
163-165
: Implement theonDiscontinuity
method to handle discontinuity eventsThe
onDiscontinuity
method is marked as not yet implemented. Implement this method to allow clients to subscribe to discontinuity events and receive a functionalUnsubscribable
for managing the subscription.Do you need help in implementing this functionality?
63-63
: Align the cancellation pattern with other SDKs to avoid confusionUsing a cancellation object named
Unsubscribable
might lead to inconsistencies across different SDKs (e.g., .NET, Go), where different patterns are used for unsubscription. Consider whether this approach aligns with the design patterns used in other Ably SDKs to maintain consistency and reduce potential confusion among users.chat-android/src/main/java/com/ably/chat/Messages.kt (2)
172-175
: Add documentation toMessagesSubscription
interface and its methodThe
MessagesSubscription
interface and thegetPreviousMessages
method lack KDoc comments. Adding documentation will enhance code readability and assist developers in understanding how to use this interface effectively.
172-175
: Consider reviewing naming conventions for subscription managementIntroducing
MessagesSubscription
and extendingUnsubscribable
may lead to confusion due to inconsistent naming across different SDKs and potential association with coroutine cancellation. As highlighted in the PR comments, consider aligning the naming conventions with other Ably SDKs (e.g., usingunsubscribe
methods) to maintain consistency and reduce confusion for developers familiar with those SDKs.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .gitignore (1 hunks)
- chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Messages.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Occupancy.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Presence.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Typing.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Unsubscribable.kt (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .gitignore
- chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt
- chat-android/src/main/java/com/ably/chat/RoomStatus.kt
Additional comments not posted (6)
chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1)
13-13
: Verify removal ofoffDiscontinuity
method usage across the codebaseEnsure that all calls to the removed
offDiscontinuity
method have been properly refactored to use the returnedUnsubscribable
(or the suggestedDisposable
) object. This verification will help prevent potential runtime errors due to lingering references to the old method.Run the following script to confirm that
offDiscontinuity
is no longer used:Verification successful
Verification successful:
offDiscontinuity
removed andonDiscontinuity
implementedThe codebase has been successfully refactored to remove the
offDiscontinuity
method and implement the newonDiscontinuity
method returning anUnsubscribable
. This change has been consistently applied across multiple files, including Messages.kt, Occupancy.kt, Presence.kt, RoomReactions.kt, and Typing.kt. The new pattern is based on theEmitsDiscontinuities
interface, ensuring a uniform approach to handling discontinuities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the removed `offDiscontinuity` method. # Test: Find all references to `offDiscontinuity`. # Expect: No matches should be found if all usages have been updated. rg --type kotlin --word-regexp '\boffDiscontinuity\b'Length of output: 55
Script:
#!/bin/bash # Verify rg is working and search for related terms # Test rg with a known term echo "Testing rg with a known term:" rg --type kotlin "fun" # Search for partial matches of "discontinuity" echo -e "\nSearching for partial matches of 'discontinuity':" rg --type kotlin "discontinuity" # Look for context of onDiscontinuity method echo -e "\nSearching for onDiscontinuity method:" rg --type kotlin -C 5 "fun.*onDiscontinuity" # Search for Unsubscribable usage echo -e "\nSearching for Unsubscribable usage:" rg --type kotlin "Unsubscribable"Length of output: 24124
chat-android/src/main/java/com/ably/chat/Occupancy.kt (2)
68-70
: Implementation ofsubscribe
method is pendingThe
subscribe
method currently contains aTODO
and is not yet implemented. This was previously noted. Please ensure the method is implemented before merging, or track this task appropriately.
76-78
: Implementation ofonDiscontinuity
method is pendingThe
onDiscontinuity
method also contains aTODO
and lacks implementation. This was previously highlighted. Please provide the implementation or create a task to address this before merging.chat-android/src/main/java/com/ably/chat/Typing.kt (2)
87-89
: Method implementation is still pendingThe
subscribe
method inDefaultTyping
remains unimplemented. Please implement this method to ensure the subscription functionality works as expected.
103-105
: Method implementation is still pendingThe
onDiscontinuity
method is not yet implemented. Refer to the previous review comments regarding its implementation.chat-android/src/main/java/com/ably/chat/Messages.kt (1)
26-26
: Ensure all usages ofsubscribe
are updated to the new signatureThe
subscribe
method now returns aMessagesSubscription
object instead ofUnit
. Please verify that all calls to this method handle the returned object appropriately to avoid any unintended behavior.Run the following script to find all usages of
subscribe
and ensure they handle the return value:
fun interface Unsubscribable { | ||
/** | ||
* Handle unsubscription (unsubscribe listeners, clean up) | ||
*/ | ||
fun unsubscribe() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming the interface for clarity
The name Unsubscribable
might be less intuitive as it's an adjective. Consider renaming the interface to Subscription
or Disposable
to better represent its purpose as an object that can be unsubscribed or disposed of. This follows common naming conventions and may enhance code readability.
* @param listener The listener | ||
*/ | ||
fun offDiscontinuity(listener: Listener) | ||
fun onDiscontinuity(listener: Listener): Unsubscribable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using Disposable
instead of Unsubscribable
for naming consistency
Using Disposable
aligns with common conventions in reactive programming frameworks in Kotlin and Java, such as RxJava, making the API more familiar to developers. This change could enhance readability and ease of understanding for users who are accustomed to these frameworks.
* @param listener A listener to be unsubscribed. | ||
*/ | ||
fun unsubscribe(listener: Listener) | ||
fun subscribe(listener: Listener): Unsubscribable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update documentation to reflect the return type of subscribe
method
The subscribe
method now returns an Unsubscribable
, but the method's documentation does not mention this return value or how it should be used by clients to manage their subscriptions. Please update the documentation to include a description of the return value and guidance on its usage.
Consider standardizing the naming of the cancellation interface
The return type Unsubscribable
may not be immediately clear to users. To improve API clarity and align with common conventions in Kotlin, consider renaming it to Cancellation
, Subscription
, or Disposable
. This change could enhance understanding and consistency across the codebase.
override fun subscribe(listener: RoomReactions.Listener): Unsubscribable { | ||
TODO("Not yet implemented") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement the subscribe
method
The subscribe
method currently contains TODO
. Please provide the implementation to ensure the functionality works as expected.
override fun onDiscontinuity(listener: EmitsDiscontinuities.Listener): Unsubscribable { | ||
TODO("Not yet implemented") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement the onDiscontinuity
method
The onDiscontinuity
method is not yet implemented. Please provide the implementation to ensure the functionality is complete.
* @param listener listener to unsubscribe | ||
*/ | ||
fun unsubscribe(listener: Listener) | ||
fun subscribe(listener: Listener): Unsubscribable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming Unsubscribable
to Subscription
for better clarity
Returning an object named Unsubscribable
may not be as intuitive as Subscription
, which is a commonly used term for a subscription handle that can be unsubscribed. This change could improve code readability and align with standard naming conventions in Kotlin and other SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why @coderabbitai is recommending to rename Unsubscribable
to either subscription
or disposable
. IMO, new renaming looks okay. It's a commonly used term across ably SDKs and easy to understand.
…th coroutine cancellation
5ef6411
to
f5b2078
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (7)
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
Line range hint
1-1
: Reconsider suppressingNotImplementedDeclaration
warningSuppressing the
NotImplementedDeclaration
warning may lead to incomplete implementations being overlooked. It's recommended to address theTODO("Not yet implemented")
placeholders or keep the warning active to ensure that unfinished methods are properly tracked.chat-android/src/main/java/com/ably/chat/Presence.kt (2)
159-160
: Implementsubscribe
method to return a validSubscription
.The
subscribe
method inDefaultPresence
currently contains a TODO. To fully adopt the new subscription pattern, please implement this method to return a validSubscription
object that manages the listener for presence events.Would you like assistance in implementing this method or should I open a new GitHub issue to track this task?
163-164
: ImplementonDiscontinuity
method to return a validSubscription
.The
onDiscontinuity
method now returns aSubscription
but is not yet implemented. Ensure that this method provides the necessary functionality to handle discontinuity events and returns a validSubscription
object.Can I assist with implementing this method or open a GitHub issue to ensure it gets addressed?
chat-android/src/main/java/com/ably/chat/Messages.kt (3)
26-26
: Ensure consistency in method documentation forsubscribe
The
subscribe
method now returns aMessagesSubscription
object. Please update the method's documentation to reflect this change, explaining the purpose of the returned object and how it should be used.
172-174
: Add documentation toMessagesSubscription
interfaceTo maintain consistency and enhance readability, please add KDoc comments to the
MessagesSubscription
interface and its methodgetPreviousMessages
, describing their functionality and usage.
173-173
: Align method naming betweengetPreviousMessages
andget
The
MessagesSubscription
interface definesgetPreviousMessages
, while theMessages
interface provides aget
method for fetching messages withQueryOptions
. Consider unifying the method names or clearly distinguishing their purposes to enhance API consistency.chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1)
13-13
: Update the method documentation to reflect the new return typeThe
onDiscontinuity
method now returns aSubscription
, but the method's documentation does not mention this change. Please update the documentation to explain what theSubscription
represents and how it should be used to manage the listener's lifecycle.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Messages.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Occupancy.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Presence.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomStatus.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Subscription.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Typing.kt (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- chat-android/src/main/java/com/ably/chat/ConnectionStatus.kt
- chat-android/src/main/java/com/ably/chat/RoomStatus.kt
Additional comments not posted (9)
chat-android/src/main/java/com/ably/chat/Subscription.kt (1)
7-12
: Interface definition is appropriateThe
Subscription
interface is well-defined, and the use offun interface
is suitable here, allowing for lambda implementations of unsubscription logic.chat-android/src/main/java/com/ably/chat/Typing.kt (3)
40-40
: Change to returnSubscription
fromsubscribe
method looks good.This update simplifies the subscription management by returning a
Subscription
object, aligning with modern API design practices.
87-89
: Implementation ofsubscribe
method is pending.The previous comment regarding this unimplemented method is still valid.
103-105
: Implementation ofonDiscontinuity
method is pending.The previous comment regarding this unimplemented method is still valid.
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (3)
114-116
: Implement thesubscribe
MethodThe
subscribe
method currently contains aTODO
and lacks an implementation. Implementing this method is essential for enabling clients to receive room-level reactions.
118-120
: Implement theonDiscontinuity
MethodThe
onDiscontinuity
method is still marked with aTODO
. Providing an implementation is necessary to handle discontinuity events properly, ensuring the reliability of the subscription mechanism.
41-41
: Ensure All Usages ofsubscribe
Handle the New Return TypeThe
subscribe
method now returns aSubscription
object. Please verify that all implementations and usages of this method have been updated to handle the new return type appropriately. This ensures that subscribers can manage their subscriptions effectively.Run the following script to find all usages of
subscribe
and check for proper handling of theSubscription
object:Verification successful
Change is Consistent and Correct Across the Codebase
The modification to return a
Subscription
object from thesubscribe
method has been consistently applied across various components in the codebase, including RoomReactions, Typing, Presence, and others. This change appears to be part of a broader refactoring or feature implementation.While most of the
subscribe
method implementations are currently marked as "Not yet implemented" (TODO), this is expected at this stage of development. The consistency of the change suggests it was an intentional design decision.As the development progresses, the team should ensure that all TODO implementations are completed consistently, properly utilizing the
Subscription
return type.The review comment has been appropriately addressed, and no issues were found with the handling of the new return type.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the subscribe method and check for handling of the Subscription return type. # Search for usages of the subscribe method in Kotlin files. rg --type kotlin '\.subscribe\(' -A 3 # Check for cases where the returned Subscription is not used. rg --type kotlin '\.subscribe\(' -A 1 | rg -B 1 -A 1 -v 'val\|var\|=\|return'Length of output: 119
Script:
#!/bin/bash # Description: Verify the existence of RoomReactions.kt and search for any occurrences of "subscribe" in Kotlin files. # Check if RoomReactions.kt exists and show its content echo "Checking RoomReactions.kt:" cat chat-android/src/main/java/com/ably/chat/RoomReactions.kt echo -e "\n\nSearching for 'subscribe' in Kotlin files:" rg --type kotlin 'subscribe' -C 3 echo -e "\n\nSearching for 'Subscription' type usage:" rg --type kotlin 'Subscription' -C 3Length of output: 30607
chat-android/src/main/java/com/ably/chat/Presence.kt (1)
63-63
: Updatedsubscribe
method to returnSubscription
.The
subscribe
method now returns aSubscription
, aligning with the new subscription model. This change enhances the API by allowing listeners to unsubscribe using the returnedSubscription
object.chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt (1)
13-13
: Ensure thatSubscription
is properly defined and consistent with conventionsVerify that the
Subscription
interface or class is defined within the codebase and provides the necessary functionality for managing subscriptions, such as anunsubscribe
orcancel
method. Additionally, consider whether usingDisposable
orAutoCloseable
might be more aligned with Kotlin and Java conventions for resource management.
In this PR, I refactored the subscription pattern to align with the JavaScript SDK. Initially, I thought having two separate methods for subscribing and unsubscribing would be more idiomatic for Kotlin/Java. However, it seems that modern libraries have started adopting the cancellation object approach.
Now, instead of using two methods, a cancellation object is returned upon subscription, which can be used to unsubscribe. This simplifies the API and introduces a unified cancellation interface, making it consistent across the codebase. It also lays the groundwork for adding convenient features later, like the
use
method in Kotlin for cleaner syntax and resource management.Summary by CodeRabbit
New Features
Subscription
interface for managing subscriptions and event listeners, enhancing resource management.Subscription
object, allowing for easier cancellation of subscriptions.Bug Fixes
unsubscribe
methods across multiple interfaces to streamline listener management.Refactor
ConnectionStatus
,EmitsDiscontinuities
, and others.