-
Notifications
You must be signed in to change notification settings - Fork 87
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 handle ownership again #876
Conversation
Pull Request Test Coverage Report for Build 3185032259
💛 - Coveralls |
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 can get rid of the ints by using an Expando
. Might be worth exploring. But otherwise LGTM.
|
||
final realm = getRealm(config); | ||
|
||
expect(() => people.length, throws<RealmClosedError>()); |
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.
Maybe have to check also people.isManaged
and people.isValid
. They should't throw, right?
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.
Do you think we may say something in the changelog about this change?
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.
There are many places from the existing code that require two arguments, but only one is passed. Is it only on my local machine or it is because of the merge?
Please make a search for the following constructors:
RealmResultsHandle._
RealmObjectHandle._
RealmListHandle._
This should be purely internal, but I can add something to the internal section.
Yeah, that happened when I merged from master via the Github UI. Will fix it before merging. |
Maybe this PR is the correct place to remove all the |
The |
Co-authored-by: Desislava Stefanova <[email protected]>
This is a refactoring of our native handle hierarchy. A summary of the changes:
HandleBase<T>
release
method is a no-op.RootedHandleBase<T>
that is owned by a Realm and it's lifetime is at most as long as the lifetime of the parent Realm.RootedHandleBase
and modifies their ctors to optionally pass in their parent Realm as root in case it's unowned.RealmHandle
cleans up all its children when released.Supersedes #528. I'd like to add more meaningful tests but those need to wait until the migrations PR is in.
Fixes #527
Fixes #837