-
Notifications
You must be signed in to change notification settings - Fork 95
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 support for string based/dynamic API on RealmObject #853
Conversation
@@ -30,7 +30,7 @@ import 'results.dart'; | |||
/// added to or deleted from the collection or from the Realm. | |||
/// | |||
/// {@category Realm} | |||
abstract class RealmList<T extends Object> with RealmEntity implements List<T>, Finalizable { | |||
abstract class RealmList<T extends Object?> with RealmEntity implements List<T>, Finalizable { |
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 liberalizes the type args on RealmList
to support nullable objects. It doesn't actually add support for lists of nullables - that is done in #708, but is necessary to simplify a lot of the generic constraints elsewhere.
return ref.values.boolean != 0; | ||
return ref.values.boolean; |
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 must be a change with the new ffi and seems like it was a bug - not sure how/why our tests didn't catch it, but likely warrants an investigation.
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.
added a test and fixed it in #854. This can be left here as well
for (var isDynamic in [true, false]) { | ||
Realm getDynamicRealm(Realm original) { | ||
Realm _getDynamicRealm(Realm original) { |
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 changes in this file are mostly whitespace due to grouping tests together. I recommend reviewing with ?w=1
.
return ref.values.boolean != 0; | ||
return ref.values.boolean; |
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.
added a test and fixed it in #854. This can be left here as well
|
||
import 'list.dart'; | ||
import 'native/realm_core.dart'; | ||
import 'realm_class.dart'; | ||
|
||
typedef DartDynamic = dynamic; |
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 suggest we don't hide this under our own name
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 is just a typedef - it's not going to affect users. The reason why I need to typedef it is to avoid a collision with the dynamic
property on RealmObject
.
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.
Won't users be hit by the same?
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 guess we can add more tests for the new validations
Adds a string-based API to read properties of a RealmObject. Right now we only support reading as that's required to get migrations working, but it can be extended in the future to also support setting.
Additionally, implements
noSuchMethod
onRealmObject
in order to support dynamic invocation:Part of #70.
Note: This only adds support for reading properties, but not for setting them. It won't be difficult to extend the API and support setting of properties, but is out of scope for this PR.