-
Notifications
You must be signed in to change notification settings - Fork 0
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.
Looks good! Although we should probably wait for Yavor's library change to be merged first and then rework this to fit the new library structure.
* Implement creating realm app CPP. * Implement user-related sync functions CPP. * Implement RealmUser Lua. * Implement RealmCredentials Lua. * Implement RealmApp and opening synced realm Lua. * Add example usage of realm sync Lua.
# Conflicts: # CMakeLists.txt # lib/realm/classes.lua # main.lua # src/CMakeLists.txt # src/realm_native_lib.cpp
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! Some minor comments
---@return Realm.App.User | nil | ||
function App:currentUser() | ||
local userHandle = native.app_get_current_user(self._handle) | ||
return RealmUser._new(userHandle) |
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 that for consistency we should write return RealmUser:new(userHandle)
, either that or change our other new
methods to _new
if it's about indicating that they're for private use.
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 we should stick to :new
. For now I don't think we necessarily expect users to access these object new functions directly but I don't see a reason to make it private as it could be nice to make them public as a more object oriented API.
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 agree that we should make changes to other :new
methods into ._new
for those that (1) are for private use, and (2) do not need self
. If there are other such methods it'd be good to make that in another PR.
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.
Gagik, didn't see your comment when I left mine. But here's an update:
The new
method for RealmUser
is never meant to be used by users. Same thing for e.g. RealmObject
. I'm not entirely against having :new
even though they're private, but I am more for indicating that it's a non-public api by using the _new
instead. As for what is the conventional Lua way.. that is a harder thing to answer :p
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 wouldn't be horrible to expose RealmObject:new
, I think it's pretty simple syntax that some usecases could prefer... of course we can't know until those usecases come to life... Regardless even if we consider them private I think we should stick with :new
just to be consistent with our object constructors
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.
Cool stuff we got 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.
Cool!
(Initially stubbed out use for sync on Lua and CPP side.)
Also adds the implementation to support Atlas Device Sync: