-
Notifications
You must be signed in to change notification settings - Fork 3
Add IDL of Objects features #345
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
6524bc0 to
0aa15d6
Compare
38a7dfa to
875d7ac
Compare
| class RealtimeObjects: // RTO* | ||
| getRoot() => io LiveMap // RTO1 | ||
|
|
||
| class LiveObject: // RTLO* |
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.
| class LiveObject: // RTLO* | |
| class BaseRealtimeObject: // RTLO* |
Since both LiveMap and LiveCounter extend this class and are part of RealtimeObjects, it makes sense to name the class BaseRealtimeObject.
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.
Additionally, we agreed to avoid using LiveObject/LiveObjects in the code.
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 agreed to not use term "LiveObjects" (plural) in the code specifically when talking about the feature "Objects".
"LiveObject" (singular) is a name for any "live object" - "live map", "live counter", "live list" (in the future)
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 is way easier to call something a "live object" (as we do in our conversations) and refer to it as "LiveObject" in the spec.
Since it also looks like that it shouldn't be necessary to expose LiveObject type to the public (kotlin and - and swift too I believe - implementations don't expose it) I'd prefer we stick to a simple name in the spec
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.
Then better if we call it BaseLiveObject or BaseObject since it's extended by LiveMap, LiveCounter and not exposed externally. wdyt
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.
But... why? I don't see a clear reason to change it. If it's not a public type then the name itself is not that important, and LiveObject type name accomplishes what we need it to do in the spec. And if it is exposed publicly (which it currently is in ably-js) we can't just change the name in the spec as we would also need to update ably-js implementation.
I really don't want to go through all spec PRs and update the LiveObject name to something else given it does what it need to right now
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.
Okay, since it's an internal type, we can ignore the naming convention.
sacOO7
left 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.
lgtm
0aa15d6 to
84fc685
Compare
Co-authored-by: Lawrence Forooghian <53756884+lawrence-forooghian@users.noreply.github.com>
875d7ac to
7ccb26d
Compare
No description provided.