-
Notifications
You must be signed in to change notification settings - Fork 26
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
[SDK-4009] Refactoring presence #1870
Conversation
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 feel most of the presence specific code is still defined under Source/ARTRealtimeChannel.m
. It should be moved under Source/ARTRealtimePresence.m
. Some of those methods are sync
, onsync
and onPresence
. Also, part of the code under setAttached
should be moved to Source/ARTRealtimePresence.m
as an individual method.
Can you please refer to https://github.com/ably/ably-go/blob/main/ably/realtime_channel.go file to check what should be defined under ARTRealtimeChannel.m
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.
These changes alone don't seem to resolve #1848; the PR doesn’t "re-enable presence tests", nor “go through the current presence implementation and check if it's compatible with spec” (although I'm not really sure what @sacOO7 had in mind when he wrote the latter; was there some reason to believe that we are not conforming to the spec)?
ac1152d
to
3bdd244
Compare
I've changed the issue to #1875. |
Changes look good to me — will let @sacOO7 say what other changes he'd like to see as part of this refactor. |
I'm working on it - move all code with |
240f979
to
dcdd823
Compare
dcdd823
to
ab104bc
Compare
77cc141
to
db461a8
Compare
[SDK-4015] Renamed presence map's `localMembers` to `internalMembers`
e365faa
to
b301249
Compare
…uick identification upon failure).
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.
Few more refactorings to be done
…e-tests [ECO-4228] Re-enable skipped presence tests
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
Closes #1875
See commit messages on what goes where.