[ECO-5577] Update method names in line with Swift guidelines #379
Conversation
WalkthroughThis PR standardizes external parameter labels across Messages, Presence, Reactions, and Subscription APIs to use explicit withX/forX labels. Call sites in the example app, mocks, and tests are updated accordingly. A small logic tweak defaults reaction count to 1 when sending a multiple-type message reaction without an explicit count. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Messages as Messages
participant Reactions as MessageReactions
participant Impl as DefaultMessageReactions
participant Ably as Ably Channel
Caller->>Messages: reactions.send(forMessageWithSerial:serial, with params)
Messages->>Reactions: send(forMessageWithSerial:serial, params)
Reactions->>Impl: send(forMessageWithSerial:serial, params)
rect rgba(200,230,255,0.3)
note over Impl: If params.type == .multiple and count == nil<br/>then set count = 1
end
Impl->>Ably: publish(reaction DTO)
Ably-->>Impl: ack or error
Impl-->>Caller: return or throw ARTErrorInfo
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lawrence-forooghian
force-pushed
the
rename-public-api-to-meet-guidelines
branch
2 times, most recently
from
43880f1 to
390cac6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
Example/AblyChatExample/ContentView.swift(4 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(5 hunks)Sources/AblyChat/DefaultRoomReactions.swift(1 hunks)Sources/AblyChat/MessageReactions.swift(2 hunks)Sources/AblyChat/Messages.swift(3 hunks)Sources/AblyChat/Presence.swift(5 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Subscription.swift(2 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(5 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(12 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(8 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(8 hunks)Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (3)
Example/AblyChatExample/Mocks/MockClients.swift (3)
send(161-177)send(249-266)send(322-333)Sources/AblyChat/DefaultMessages.swift (1)
send(126-132)Sources/AblyChat/DefaultRoomReactions.swift (1)
send(18-32)
Sources/AblyChat/MessageReactions.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Sources/AblyChat/Messages.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (4)
history(157-159)send(161-177)send(249-266)send(322-333)Sources/AblyChat/DefaultMessages.swift (2)
history(118-124)send(126-132)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
Sources/AblyChat/DefaultMessages.swift (2)
send(126-132)history(118-124)Sources/AblyChat/Messages.swift (1)
getPreviousMessages(394-396)
Sources/AblyChat/Presence.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (16)
get(29-36)get(384-386)get(439-448)get(450-459)get(567-569)isUserPresent(461-463)enter(465-467)enter(469-471)enter(473-486)update(179-192)update(488-490)update(492-494)update(496-509)leave(511-513)leave(515-517)leave(519-532)Sources/AblyChat/DefaultPresence.swift (12)
get(20-43)get(45-68)isUserPresent(71-95)enter(97-99)enter(101-103)enter(106-127)update(129-131)update(133-135)update(138-159)leave(161-163)leave(165-167)leave(170-191)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Sources/AblyChat/DefaultMessages.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (4)
history(157-159)send(161-177)send(249-266)send(322-333)Sources/AblyChat/ChatAPI.swift (1)
getMessages(19-22)
Sources/AblyChat/Subscription.swift (1)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
historyBeforeSubscribe(211-213)
Sources/AblyChat/DefaultRoomReactions.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (3)
send(161-177)send(249-266)send(322-333)Sources/AblyChat/DefaultMessages.swift (1)
send(126-132)
Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (1)
Sources/AblyChat/Messages.swift (1)
getPreviousMessages(394-396)
Example/AblyChatExample/ContentView.swift (7)
Example/AblyChatExample/Mocks/MockClients.swift (8)
enter(465-467)enter(469-471)enter(473-486)send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultPresence.swift (3)
enter(97-99)enter(101-103)enter(106-127)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
historyBeforeSubscribe(211-213)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)Sources/AblyChat/DefaultMessages.swift (2)
send(126-132)delete(142-148)Sources/AblyChat/DefaultRoomReactions.swift (1)
send(18-32)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (4)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)Sources/AblyChat/DefaultMessages.swift (2)
send(126-132)delete(142-148)Sources/AblyChat/DefaultRoomReactions.swift (1)
send(18-32)
Sources/AblyChat/DefaultMessageReactions.swift (1)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)
Sources/AblyChat/RoomReactions.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (3)
send(161-177)send(249-266)send(322-333)Sources/AblyChat/DefaultRoomReactions.swift (1)
send(18-32)
Tests/AblyChatTests/DefaultPresenceTests.swift (4)
Example/AblyChatExample/Mocks/MockClients.swift (16)
enter(465-467)enter(469-471)enter(473-486)update(179-192)update(488-490)update(492-494)update(496-509)leave(511-513)leave(515-517)leave(519-532)isUserPresent(461-463)get(29-36)get(384-386)get(439-448)get(450-459)get(567-569)Sources/AblyChat/DefaultPresence.swift (12)
enter(97-99)enter(101-103)enter(106-127)update(129-131)update(133-135)update(138-159)leave(161-163)leave(165-167)leave(170-191)isUserPresent(71-95)get(20-43)get(45-68)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (5)
update(292-306)leave(260-274)get(224-240)get(242-258)get(454-457)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (4)
update(53-58)leave(39-44)get(23-29)get(31-37)
Tests/AblyChatTests/IntegrationTests.swift (6)
Example/AblyChatExample/Mocks/MockClients.swift (20)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)enter(465-467)enter(469-471)enter(473-486)get(29-36)get(384-386)get(439-448)get(450-459)get(567-569)update(179-192)update(488-490)update(492-494)update(496-509)leave(511-513)leave(515-517)leave(519-532)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)Sources/AblyChat/DefaultMessages.swift (3)
send(126-132)delete(142-148)update(134-140)Sources/AblyChat/DefaultRoomReactions.swift (1)
send(18-32)Sources/AblyChat/Messages.swift (1)
getPreviousMessages(394-396)Sources/AblyChat/DefaultPresence.swift (11)
enter(97-99)enter(101-103)enter(106-127)get(20-43)get(45-68)update(129-131)update(133-135)update(138-159)leave(161-163)leave(165-167)leave(170-191)
Sources/AblyChat/DefaultPresence.swift (1)
Example/AblyChatExample/Mocks/MockClients.swift (16)
get(29-36)get(384-386)get(439-448)get(450-459)get(567-569)isUserPresent(461-463)enter(465-467)enter(469-471)enter(473-486)update(179-192)update(488-490)update(492-494)update(496-509)leave(511-513)leave(515-517)leave(519-532)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (28)
Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (1)
55-55: LGTM!The test correctly uses the new
withParamsparameter label, aligning with the protocol changes.Sources/AblyChat/DefaultMessages.swift (2)
118-124: LGTM!The
withOptionsparameter label improves API clarity and follows Swift naming conventions for methods that accept option-style parameters.
126-132: LGTM!The
withParamsparameter label improves API clarity and follows Swift naming conventions.Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
24-24: LGTM!The test correctly uses the updated
withParamsparameter label.Sources/AblyChat/DefaultRoomReactions.swift (1)
18-32: LGTM!The implementation correctly adopts the
withParamsparameter label, matching the updated protocol signature.Sources/AblyChat/DefaultMessageReactions.swift (3)
22-22: LGTM!The
forMessageWithSerialparameter label improves API clarity by making it explicit that the reaction is being sent for a specific message.
24-27: Good fix: Implements the documented default behavior.This change ensures that when sending a
multiple-type reaction without an explicit count, it defaults to 1. This aligns with the API documentation inMessageReactions.swift(line 136) which states "Defaults to 1 if not set."This is a behavior fix, not just a parameter label change, and should be highlighted in the PR description or release notes.
43-59: LGTM!The
forMessageWithSerialparameter label improves API clarity for the delete operation.Sources/AblyChat/MessageReactions.swift (2)
22-22: LGTM!The
forMessageWithSerialparameter label makes the API more expressive and follows Swift conventions for methods that operate on a specific resource.
31-31: LGTM!The
forMessageWithSerialparameter label provides consistency with thesendmethod and improves API clarity.Sources/AblyChat/Subscription.swift (1)
94-109: LGTM!The implementation correctly adopts the
withParamsparameter label, matching the updated protocol signature. The internal logic remains unchanged and correct.Example/AblyChatExample/Mocks/MockClients.swift (1)
249-249: LGTM! Consistent mock API alignment.The mock implementation correctly adopts the new
forMessageWithSerialexternal parameter label for bothsendanddeletemethods, maintaining consistency with the production implementation.Also applies to: 268-268
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
39-39: LGTM! Comprehensive test updates.All test call sites have been consistently updated to use the new explicit parameter labels (
withParams,withOptions). The test logic remains unchanged, ensuring that the renamed methods continue to work as expected.Also applies to: 163-163, 235-235, 262-262, 289-289, 304-304, 331-331, 343-343, 371-371, 413-413, 439-439, 470-470
Sources/AblyChat/Messages.swift (1)
36-36: LGTM! Clear parameter label improvements.The protocol declarations now use explicit parameter labels (
withOptions,withParams) that make the API more self-documenting and align with Swift API Design Guidelines for "grammatical English phrases."Also applies to: 50-50, 394-394
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
26-28: LGTM! Reaction tests properly updated.All message reaction test scenarios have been updated to use the new explicit parameter labels. The tests maintain coverage of both normal flows and edge cases (empty serials, default reaction types).
Also applies to: 73-73, 96-96, 348-349, 387-388
Sources/AblyChat/Presence.swift (1)
32-32: LGTM! Consistent presence API improvements.The Presence protocol methods now use explicit parameter labels (
withParams,withClientID,withData) that create grammatical English phrases and improve API clarity. The naming is consistent across all presence operations.Also applies to: 44-44, 54-54, 64-64, 74-74
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
25-25: LGTM! Complete presence test coverage.All presence test scenarios have been consistently updated to use the new explicit parameter labels. The tests maintain comprehensive coverage of presence operations including enter, update, leave, get, and user presence checks.
Also applies to: 170-170, 290-290, 318-318, 347-347, 376-376, 402-402, 433-433
Example/AblyChatExample/ContentView.swift (1)
227-227: LGTM! Example app demonstrates clear API usage.The example app has been updated to use the new explicit parameter labels across all features (presence, messages, reactions). The updated API calls read as grammatical English phrases (e.g.,
enter(withData:),send(withParams:)), demonstrating the improved clarity of the API.Also applies to: 278-278, 390-390, 420-420, 426-426, 432-432
Sources/AblyChat/DefaultPresence.swift (1)
45-45: LGTM! Implementation matches protocol signatures.The internal implementations correctly adopt the new explicit parameter labels while maintaining the existing logic and internal helper methods. The changes are purely cosmetic at the API boundary level.
Also applies to: 71-71, 97-97, 129-129, 161-161
Tests/AblyChatTests/IntegrationTests.swift (9)
113-117: LGTM!The updated method call correctly uses the new
withParams:external parameter label, which aligns with Swift API Design Guidelines by forming a grammatical English phrase.
127-133: LGTM!The method call correctly uses
withParams:and maintains all parameter values (text, metadata, headers) from the original implementation.
143-146: LGTM!The message reaction calls correctly use the new
forMessageWithSerial:label, which improves clarity by explicitly indicating that reactions are targeting a specific message serial.
191-193: LGTM!Consistent with the earlier reaction calls, these operations correctly use the
forMessageWithSerial:label for clarity.
224-224: LGTM!Both
getPreviousMessagescalls correctly use the newwithParams:label, including the commented-out line and the actual implementation in the retry loop.Also applies to: 242-242
325-331: LGTM!The room reaction send call correctly uses
withParams:, following the same pattern as the message send calls.
383-383: LGTM!The presence calls correctly use the new labels:
enter(withData:)andget(withParams:), which form grammatical English phrases as per Swift guidelines.Also applies to: 389-389
394-394: LGTM!The presence
update(withData:)andleave(withData:)calls follow the same pattern asenter(withData:), providing consistency across all presence mutation operations.Also applies to: 400-400
406-406: LGTM! Comprehensive and consistent updates.The remaining presence operations on rxRoom correctly use the
withData:label. All affected call sites in the integration test have been successfully updated to align with the renamed public API. The changes maintain consistency across:
- Messages:
send(withParams:)- Message Reactions:
send/delete(forMessageWithSerial:params:)- Message Subscriptions:
getPreviousMessages(withParams:)- Room Reactions:
send(withParams:)- Presence:
enter/update/leave(withData:),get(withParams:)Also applies to: 412-412, 418-418
lawrence-forooghian
force-pushed
the
rename-public-api-to-meet-guidelines
branch
from
390cac6 to
21886f5
Compare
See [1], in particular "Prefer method and function names that make use sites form grammatical English phrases". The "omit needless words" guideline is a bit harder to interpret and I wonder whether, under that guideline, things like `send(withParams params: SendMessageParams)` should be `with params: SendMessageParams` but honestly I don't know how understandable that is, especially when you use implicit `.init(…)` to create the argument. I think better to err on the side of caution and being a bit more verbose? Have renamed: - Messages: - history(options: QueryOptions) to history(withOptions options: QueryOptions) - send(params: SendMessageParams) to send(withParams params: SendMessageParams) - MessageSubscriptionAsyncSequence: - getPreviousMessages(params: QueryOptions) to getPreviousMessages(withParams params: QueryOptions) - MessageReactions: - send(messageSerial: String, params: SendMessageReactionParams) to send(forMessageWithSerial messageSerial: String, params: SendMessageReactionParams) - delete(messageSerial: String, params: SendMessageReactionParams) to delete(forMessageWithSerial messageSerial: String, params: SendMessageReactionParams) - Presence: - get(params: PresenceParams) to get(withParams params: PresenceParams) - isUserPresent(clientID: String) to isUserPresent(withClientID clientID: String) - enter(data: PresenceData) to enter(withData data: PresenceData); ditto for update, leave - RoomReactions: - send(params: SendReactionParams) to send(withParams params: SendReactionParams) - MessageSubscriptionResponse: - historyBeforeSubscribe(_ params: QueryOptions) to historyBeforeSubscribe(withParams params: QueryOptions) Have not touched Messages' `update` or `delete` methods yet because their signature seems completely inconsistent with that of JS; need to look into it further. [1] https://www.swift.org/documentation/api-design-guidelines/
lawrence-forooghian
force-pushed
the
rename-public-api-to-meet-guidelines
branch
from
21886f5 to
c32a2fa
Compare
lawrence-forooghian
changed the title
lawrence-forooghian
changed the title
See https://www.swift.org/documentation/api-design-guidelines/, in particular "Prefer method and function names that make use sites form grammatical English phrases".
The "omit needless words" guideline is a bit harder to interpret and I wonder whether, under that guideline, things like
send(withParams params: SendMessageParams)should bewith params: SendMessageParamsbut honestly I don't know how understandable that is, especially when you use implicit.init(…)to create the argument. I think better to err on the side of caution and being a bit more verbose?Have renamed:
Messages:history(options: QueryOptions)tohistory(withOptions options: QueryOptions)send(params: SendMessageParams)tosend(withParams params: SendMessageParams)MessageSubscriptionAsyncSequence:getPreviousMessages(params: QueryOptions)togetPreviousMessages(withParams params: QueryOptions)MessageReactions:send(messageSerial: String, params: SendMessageReactionParams)tosend(forMessageWithSerial messageSerial: String, params: SendMessageReactionParams)delete(messageSerial: String, params: SendMessageReactionParams)todelete(forMessageWithSerial messageSerial: String, params: SendMessageReactionParams)Presence:get(params: PresenceParams)toget(withParams params: PresenceParams)isUserPresent(clientID: String)toisUserPresent(withClientID clientID: String)enter(data: PresenceData)toenter(withData data: PresenceData); ditto forupdate,leaveRoomReactions:send(params: SendReactionParams)tosend(withParams params: SendReactionParams)MessageSubscriptionResponse:historyBeforeSubscribe(_ params: QueryOptions)tohistoryBeforeSubscribe(withParams params: QueryOptions)Have not touched Messages'
updateordeletemethods yet because their signature seems completely inconsistent with that of JS; need to look into it further.Summary by CodeRabbit
Refactor
Bug Fixes
Tests