Update duplicate spec points #95
Conversation
The duplicate spec points were renamed in commit 0e5ab98 of [1]. The currently-implemented room lifecycle behaviour (I can’t speak for the messages functionality) now conforms to that commit of the spec. [1] ably/specification#200.
WalkthroughThe changes in this pull request involve modifications to the Changes
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: 2
🧹 Outside diff range and nitpick comments (3)
Tests/AblyChatTests/RoomLifecycleManagerTests.swift (3)
Line range hint
705-705: Remove unnecessaryawaitinasync letbindingWhen using
async letto initiate an asynchronous task, theawaitkeyword should not be used on the right-hand side. Theawaitis only necessary when retrieving the result of the asynchronous task later in your code. Therefore, the line:async let _ = await manager.performReleaseOperation()should be corrected to:
async let _ = manager.performReleaseOperation()This ensures proper usage of
async letwithout prematurely awaiting the result.Apply this diff to fix the issue:
- async let _ = await manager.performReleaseOperation() + async let _ = manager.performReleaseOperation()
Line range hint
590-590: Rename variable for clarityThe variable name
asyncLetStatusChangesmay be unclear and could confuse readers about its purpose. Consider renaming it tostatusChangesorstatusChangesArrayto improve readability and accurately reflect its contents.Apply this diff to improve variable naming:
- async let asyncLetStatusChanges = Array(statusChangeSubscription.prefix(2)) + async let statusChanges = Array(statusChangeSubscription.prefix(2))
Line range hint
464-472: Avoid unnecessary external state tracking in closuresIn the
attach_whenChannelDetachTriggered_ifADetachFailsItIsRetriedUntilSuccesstest, thedetachResultclosure uses thecallCountparameter to track the number of times it has been called:let detachResult = { @Sendable (callCount: Int) async -> MockRoomLifecycleContributorChannel.AttachOrDetachResult in if callCount == 1 { return .failure(.create(withCode: 123, message: "")) } else { return .success } }However, tracking
callCountexternally is unnecessary. You can utilize the inherent properties of the closure or use aSignallableChannelOperationto manage completion results without additional state variables.Refactor the closure to eliminate external state tracking:
let contributorDetachOperation = SignallableChannelOperation() var detachAttempts = 0 let detachBehavior = MockRoomLifecycleContributorChannel.AttachOrDetachBehavior.fromFunction { _ in detachAttempts += 1 if detachAttempts == 1 { return .failure(.create(withCode: 123, message: "")) } else { return .success } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Tests/AblyChatTests/RoomLifecycleManagerTests.swift(2 hunks)
🧰 Additional context used
📓 Learnings (2)
Sources/AblyChat/RoomLifecycleManager.swift (4)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:0-0
Timestamp: 2024-09-18T18:34:37.252Z
Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:0-0
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Sources/AblyChat/RoomLifecycleManager.swift:217-220
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Sources/AblyChat/RoomLifecycleManager.swift:217-220
Timestamp: 2024-10-01T19:44:34.032Z
Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.
Tests/AblyChatTests/RoomLifecycleManagerTests.swift (8)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-10-08T15:58:47.376Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-10-01T12:55:21.968Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-09-23T19:56:39.435Z
Learning: In Swift Testing, test method names do not need to start with `test`.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In Swift Testing, test method names do not need to start with `test`.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941
Timestamp: 2024-10-01T19:46:16.467Z
Learning: When a TODO comment references an existing GitHub issue, it's acceptable to leave it in the code without requesting its removal.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941
Timestamp: 2024-10-08T15:58:47.376Z
Learning: When a TODO comment references an existing GitHub issue, it's acceptable to leave it in the code without requesting its removal.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836
Timestamp: 2024-10-01T19:46:04.461Z
Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836
Timestamp: 2024-10-08T15:58:47.376Z
Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.
🔇 Additional comments (2)
Sources/AblyChat/DefaultMessages.swift (1)
54-54: LGTM: Specification point update matches implementation.
The update from CHA-M5d to CHA-M5k aligns with the Ably specification changes. The implementation correctly handles malformed events by validating required fields (text, extras, timeserial, clientID) and throwing appropriate errors, ensuring malformed events are not emitted to subscribers.
Let's verify this change aligns with the specification update:
✅ Verification successful
Specification point update verified and consistent
The verification confirms:
- The old specification point
CHA-M5dhas been completely removed from the codebase - The new specification point
CHA-M5kappears exactly once in the expected location - The change maintains specification consistency across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the specification point update across the codebase
# Expected: Only one occurrence of CHA-M5k, no occurrences of CHA-M5d
echo "Checking for old spec point (should be 0 results):"
rg "CHA-M5d"
echo "Checking for new spec point (should be 1 result):"
rg "CHA-M5k"
Length of output: 420
Tests/AblyChatTests/RoomLifecycleManagerTests.swift (1)
Line range hint 653-698: Ensure consistent use of async let without await
In the release_whenReleasing test, you correctly use async let without await when initiating the first release operation:
async let _ = manager.performReleaseOperation(testsOnly_forcingOperationID: firstReleaseOperationID)However, ensure this practice is consistent throughout your tests to prevent confusion and maintain code consistency.
lawrence-forooghian
force-pushed
the
address-duplicate-spec-points
branch
from
b539224 to
20f21c7
Compare
|
@umair-ably @maratal I’m going to merge this one so that I can merge #87; I don't think there's anything controversial here. |
The duplicate spec points were renamed in ably/specification@0e5ab98.
Summary by CodeRabbit
New Features
Bug Fixes
Tests