[ECO-5168] Rename subscribeToDiscontinuities to onDiscontinuity
#183
Conversation
I remember that when I wrote the public API in 20e7f5f, I was a bit reluctant to copy JS’s `on*` naming, because that read to me more like an API that was designed for receiving a listener (i.e. “_on_ this event, call this listener”). But, I think that after some back and forth with Andy, we decided to stick to the `on*` naming, hence `onStatusChange`. For some reason, though, I didn’t apply this decision to `subscribeToDiscontinuties`. So, do that. Resolves #173.
WalkthroughThe changes in this pull request primarily involve renaming the method Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (3)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)
Line range hint
7-12: Fix inconsistent variable naming in initializerThere is a mismatch between the property and the initializer parameter names:
- Property:
resultOfWaitToBeAbleToPerformPresenceOperations- Parameter:
resultOfWaitToBeAblePerformPresenceOperations(missing "To")As a result, the property is not correctly assigned, leading to potential issues.
Apply this diff to correct the parameter name and ensure proper assignment:
init( channel: RealtimeChannelProtocol, - resultOfWaitToBeAblePerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil + resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil ) { self.channel = channel - resultOfWaitToBeAblePerformPresenceOperations = resultOfWaitToBeAblePerformPresenceOperations + self.resultOfWaitToBeAbleToPerformPresenceOperations = resultOfWaitToBeAbleToPerformPresenceOperations }
Line range hint
25-27: Correct variable name in error messageIn the
waitToBeAbleToPerformPresenceOperationsmethod, thefatalErrormessage refers toresultOfWaitToBeAblePerformPresenceOperations(missing "To"), which is inconsistent with the property nameresultOfWaitToBeAbleToPerformPresenceOperations. This could cause confusion during debugging.Apply this diff to update the error message:
guard let resultOfWaitToBeAbleToPerformPresenceOperations else { - fatalError("resultOfWaitToBeAblePerformPresenceOperations must be set before waitToBeAbleToPerformPresenceOperations is called") + fatalError("resultOfWaitToBeAbleToPerformPresenceOperations must be set before waitToBeAbleToPerformPresenceOperations is called") }Sources/AblyChat/DefaultRoomReactions.swift (1)
Line range hint
3-3: Consider tracking the @mainactor requirement separately.The TODO comment suggests this is a temporary fix for task isolation. Consider creating a separate issue to properly address the underlying concurrency concern.
Would you like me to create a GitHub issue to track the proper implementation of task isolation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
Example/AblyChatExample/Mocks/MockClients.swift(5 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(1 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(1 hunks)Sources/AblyChat/DefaultRoomReactions.swift(1 hunks)Sources/AblyChat/DefaultTyping.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)
🔇 Additional comments (12)
Sources/AblyChat/DefaultPresence.swift (1)
197-198: Renamed method to onDiscontinuity for consistency
The method subscribeToDiscontinuities has been correctly renamed to onDiscontinuity, aligning with the on* event listener naming convention used in JavaScript. This enhances consistency across the API.
Sources/AblyChat/EmitsDiscontinuities.swift (1)
13-14: Updated protocol methods to onDiscontinuity
Renaming the methods in the EmitsDiscontinuities protocol and its extension to onDiscontinuity improves clarity and aligns with the event listener naming conventions, ensuring consistency throughout the codebase.
Also applies to: 17-17, 21-22
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
18-20: Renamed method to onDiscontinuity for consistency
The method subscribeToDiscontinuities has been renamed to onDiscontinuity, aligning with the on* naming convention and improving consistency with other implementations.
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1)
21-21: Method renamed to onDiscontinuity for consistency
The method subscribeToDiscontinuities has been correctly renamed to onDiscontinuity, adhering to the event listener naming convention and enhancing codebase consistency.
Sources/AblyChat/DefaultOccupancy.swift (1)
53-54: Renamed method to onDiscontinuity to align with naming conventions
The method subscribeToDiscontinuities has been appropriately renamed to onDiscontinuity, ensuring consistency with the on* event listener pattern across the API.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
66-81: LGTM! Method renaming is consistent with PR objectives.
The test method has been correctly renamed from subscribeToDiscontinuities to onDiscontinuity while maintaining the same test logic and verification steps. The comments have been appropriately updated to reflect the new method name.
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
74-90: LGTM! Method renaming maintains consistency across test files.
The test method has been correctly renamed from subscribeToDiscontinuities to onDiscontinuity, maintaining consistency with other test files. The test logic and verification steps remain unchanged, and comments have been appropriately updated.
Sources/AblyChat/RoomFeature.swift (1)
60-61: LGTM! Core implementation correctly renamed.
The method has been properly renamed from subscribeToDiscontinuities to onDiscontinuity in the DefaultFeatureChannel implementation, maintaining consistency with the test files and aligning with JavaScript naming conventions.
Let's verify that all usages of the old method name have been updated:
✅ Verification successful
All instances of the method have been properly renamed
The verification shows that:
- No instances of the old method name
subscribeToDiscontinuitiesremain in the codebase - The new method name
onDiscontinuityis consistently used across all relevant files:- Core implementation files (RoomFeature.swift, DefaultMessages.swift, etc.)
- Protocol definition (EmitsDiscontinuities.swift)
- Test files (DefaultRoomReactionsTests.swift, DefaultMessagesTests.swift)
- Mock implementations (MockFeatureChannel.swift, MockClients.swift)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no remaining instances of old method name
# and confirm consistent usage of new method name
echo "Checking for any remaining instances of old method name..."
rg "subscribeToDiscontinuities"
echo "Verifying consistent usage of new method name..."
rg "onDiscontinuity" -A 2
Length of output: 8439
Sources/AblyChat/DefaultRoomReactions.swift (1)
83-84: LGTM! Method renamed as per convention.
The renaming from subscribeToDiscontinuities to onDiscontinuity aligns with JavaScript event listener naming conventions.
Sources/AblyChat/DefaultTyping.swift (1)
163-164: LGTM! Method renamed consistently.
The renaming from subscribeToDiscontinuities to onDiscontinuity maintains consistency with the new naming convention.
Example/AblyChatExample/Mocks/MockClients.swift (1)
147-147: LGTM! Methods renamed consistently across mock classes.
The renaming from subscribeToDiscontinuities to onDiscontinuity has been applied consistently across all mock implementations.
Also applies to: 198-198, 247-247, 351-351, 386-386
Sources/AblyChat/DefaultMessages.swift (1)
112-113: LGTM! Method renaming aligns with JavaScript conventions.
The renaming from subscribeToDiscontinuities to onDiscontinuity maintains functionality while improving API consistency by following the on* event listener pattern common in JavaScript.
Let's verify the consistency of this renaming across the codebase:
✅ Verification successful
Method renaming has been consistently applied across the codebase
The verification confirms that:
- No instances of the old method name
subscribeToDiscontinuitiesremain in the codebase - The new method name
onDiscontinuityis consistently used across all relevant files:- Protocol definition in
EmitsDiscontinuities.swift - Implementations in feature classes (
DefaultMessages,DefaultTyping,DefaultPresence, etc.) - Test files and mocks
- Protocol definition in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all occurrences of the old method name have been renamed
# and the new name is used consistently across the codebase.
echo "Checking for any remaining instances of old method name..."
rg "subscribeToDiscontinuities"
echo "Verifying consistent usage of new method name..."
rg "onDiscontinuity" -A 2
Length of output: 8439
I remember that when I wrote the public API in 20e7f5f, I was a bit reluctant to copy JS’s
on*naming, because that read to me more like an API that was designed for receiving a listener (i.e. “on this event, call this listener”). But, I think that after some back and forth with Andy, we decided to stick to theon*naming, henceonStatusChange. For some reason, though, I didn’t apply this decision tosubscribeToDiscontinuties. So, do that.Resolves #173.
Summary by CodeRabbit
New Features
subscribeToDiscontinuitiestoonDiscontinuity.Bug Fixes
Documentation
Tests