-
Notifications
You must be signed in to change notification settings - Fork 44
[ECO-5468] Add Swift documentation for LiveObjects #2769
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
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
7319933
to
d2dab34
Compare
d2dab34
to
b35ffea
Compare
b35ffea
to
511a378
Compare
511a378
to
4def6fd
Compare
a1043d1
to
834e167
Compare
834e167
to
c60f671
Compare
c60f671
to
ec22885
Compare
ec22885
to
3810265
Compare
3810265
to
e0d57d2
Compare
627669c
to
4e2bf5f
Compare
4e2bf5f
to
d2f1f26
Compare
Done |
7185deb
to
555a1d6
Compare
<If lang="swift"> | ||
To remove a counter update listener from _inside_ the listener function, you can call `unsubscribe()` on the subscription response that is passed as the second argument to the listener function: | ||
|
||
```swift |
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.
This needs to be wrapped in <Code>
tags.
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.
Thanks, done
<If lang="swift"> | ||
To remove a map update listener from _inside_ the listener function, you can call `unsubscribe()` on the subscription response that is passed as the second argument to the listener function: | ||
|
||
```swift |
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.
This needs to be wrapped in <Code>
tags.
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.
Thanks, done
@@ -33,6 +33,7 @@ export default { | |||
}, | |||
liveObjects: { | |||
javascript: '2.9', | |||
swift: '0.1', |
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.
It's a little odd that we're tying JS/Java to their Pub/Sub version but having Swift at 0.1 just because of how they're imported (just a comment).
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.
The difference is that in JS and Java, the LiveObjects plugin is part of the core SDK and hence must have the same version. In Swift, we are forced to have a separate repository for the plugin, which has the happy side effect of allowing us to choose a semantic version number that reflects the fact that the LiveObjects API is experimental (JS and Java instead have to rely on code comments that state this).
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.
Yeah, I totally understand the reasoning. Just from an external perspective it makes it look like JS and Java are far more mature than the Swift implementation.
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.
For sure — do you have any thoughts on what we could do to mitigate this?
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.
I think it's something we need to look at holistically that we should include on some guidance for releasing plugins. Not worth blocking this from releasing right now.
</Code> | ||
|
||
## Update LiveCounter <a id="update"/> | ||
|
||
Update the counter value by calling `LiveCounter.increment()` or `LiveCounter.decrement()`. These operations are synchronized across all clients and trigger data subscription callbacks for the counter, including on the client making the request. | ||
Update the counter value by calling <If lang="javascript">`LiveCounter.increment()` or `LiveCounter.decrement()`</If><If lang="swift">`LiveCounter.increment(amount:)` or `LiveCounter.decrement(amount:)`</If>. These operations are synchronized across all clients and trigger data subscription callbacks for the counter, including on the client making the request. |
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.
I'd personally keep all these occurrences as LiveCounter.increment()
since the usage is immediately below to save ending up with 10+ theoretical if statements, unless you think it's that important to keep in.
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.
Could you please explain what you mean by "10+ theoretical if statements"? In Swift, the name of a method always includes its parameter labels and this is usually respected in documentation; I could omit them to keep things maintainable here but it might look a bit sloppy to people who are used to working in Swift.
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.
With 10+ I was referring to if we were to implement LiveObjects in all SDKs and they all have slightly different signatures.
I have no issues at all with keeping it, if it's the common pattern for Swift. We should extend this to other products though in that case (e.g. Chat definitely doesn't follow this format for Swift which is what made me question the need here).
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.
We should extend this to other products though in that case (e.g. Chat definitely doesn't follow this format for Swift which is what made me question the need here).
Agreed — created ably/ably-cocoa#2093 and ably/ably-chat-swift#351
555a1d6
to
eaf311a
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.
one more file conflict
I'll be adding Swift documentation for LiveObjects shortly, thus giving us our second language. Mark indicated that we wish to have a per-language quickstart guide, consistent with the other getting started guides e.g. chat.
eaf311a
to
f0598c5
Compare
Fixed |
Based on Swift plugin at commit 714988d and ably-cocoa's integration/liveobjects branch at commit 5096ca3. Some notes: - I haven't added Swift code to the batch API page because it's not yet implemented in Swift and the stub public API that we do have is incorrect and will be removed pre-v0.1 launch. - I haven't bothered adding Swift to the inband-objects page given that on the same page we're telling people to use the Swift SDK. The quickstart content is a copy and paste of the JS quickstart.
f0598c5
to
47e4dfd
Compare
@@ -0,0 +1,201 @@ | |||
--- | |||
title: "Quickstart: LiveObjects in Swift" |
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.
title: "Quickstart: LiveObjects in Swift" | |
title: "Getting started: LiveObjects in Swift" |
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.
updated in the java doc PR to avoid conflicts #2771 (comment)
Description
Based on the Swift plugin at commit
714988d
and ably-cocoa'sintegration/liveobjects
branch at commit5096ca3
.Some notes:
The quickstart content is a copy and paste of the JS quickstart.
Resolves https://ably.atlassian.net/browse/ECO-5468.
Checklist