-
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
Add no-arg variants of presence operations #186
Conversation
WalkthroughThe changes in 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 (
|
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: 0
🧹 Outside diff range and nitpick comments (1)
Sources/AblyChat/Presence.swift (1)
100-112
: Add documentation for new no-arg variantsWhile the implementation is correct, consider adding documentation comments for the new methods to explain their behavior, especially since the PR mentions that the expected behavior isn't explicitly defined in the chat specification.
Add documentation like this:
public extension Presence { + /// Enters presence on the channel without any presence data. + /// This is a convenience method that calls `enter(data: nil)`. func enter() async throws { try await enter(data: nil) } + /// Updates presence on the channel without any presence data. + /// This is a convenience method that calls `update(data: nil)`. func update() async throws { try await update(data: nil) } + /// Leaves presence on the channel without any presence data. + /// This is a convenience method that calls `leave(data: nil)`. func leave() async throws { try await leave(data: nil) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Sources/AblyChat/Presence.swift
(2 hunks)
🔇 Additional comments (2)
Sources/AblyChat/Presence.swift (2)
83-87
: LGTM: No-arg variants properly added to protocol
The new no-arg variants are correctly declared with async/throws semantics, matching the existing method patterns.
Line range hint 4-6
: Verify handling of floating-point numbers after NSNumber to Int change
The change from NSNumber
to Int
improves Codable conformance but might break existing code if floating-point values were previously stored using NSNumber.
✅ Verification successful
Let me search for any test files or usage examples to see if there are any floating-point numbers being used with the presence data.
Based on the search results, I can now generate the final response:
Change from NSNumber to Int appears safe in current usage
The codebase shows no evidence of floating-point values being used with .number
cases in PresenceCustomData. The change appears to be part of a broader pattern where similar enums (Headers, Metadata) have also moved from NSNumber to Int. No test files or actual usage patterns show floating-point number handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any floating-point number usage in presence data
rg -A 3 "\.number\("
Length of output: 848
Script:
#!/bin/bash
# Search for test files related to presence
fd ".*test.*\.swift" --exec rg -l "PresenceCustomData"
# Search for any floating-point literals in the context of presence
rg --type swift "\.number\(.*\.[0-9]+"
# Search for any NSNumber usage in tests
rg --type swift "NSNumber" -A 2
Length of output: 1127
I missed these when copying across the public API from JS in 20e7f5f. The correct behaviour of these variants is not specified by the chat spec, but this will behaviour will do for now; I’ll think about it a bit more in #178.
Summary by CodeRabbit
New Features
enter()
,update()
, andleave()
.Bug Fixes
PresenceCustomData
enum to useInt
instead ofNSNumber
.Documentation