-
Notifications
You must be signed in to change notification settings - Fork 86
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
Const schema refactoring #903
base: main
Are you sure you want to change the base?
Conversation
45bd665
to
039bac7
Compare
958b123
to
20d975d
Compare
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, I have mostly clarification comments and minor suggestions
var object = RealmObjectInternal.create<T>(this, handle, metadata); | ||
return object; |
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.
var object = RealmObjectInternal.create<T>(this, handle, metadata); | |
return object; | |
return RealmObjectInternal.create<T>(this, handle, metadata); |
|
||
return _values[name]; | ||
} | ||
T getValue<T>(RealmObject object, String propertyName) => _values[propertyName] as T; |
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.
How does that work with default values? Like if I call getValue<int>(obj, age)
and age doesn't exist in _values
, wouldn't that throw?
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.
Default values are handled at the property layer
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.
Yes, but that's happening after calling this method. Here's an example of what I'm thinking about: https://dartpad.dev/?id=4d103b52e90ef328e32c12665a38a337
7a430df
to
f94c072
Compare
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 some questions?
const _intMapping = Mapping<int>(); | ||
const _boolMapping = Mapping<bool>(); | ||
const _doubleMapping = Mapping<double>(); |
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.
Why this types are special and we have constants only for them?
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.
because int, bool, and double are both members of the enum and primitive types in dart.. Hence this little trick
I suppose even though this is a refactoring we will still have a line in the changelog about it. |
9857287
to
36370c9
Compare
f94c072
to
b027b2c
Compare
36370c9
to
92474cd
Compare
043695c
to
a6ef9fc
Compare
92474cd
to
31a2a3c
Compare
Caveat to be aware of: ```dart @RealmModel() class _Person { @PrimaryKey() late String name; int age = 42; } final p = Person('Bob', age: 42); final p2 = Person('Bob')..age = 42; // behaves slightly differently wrt. sync ```
…allows frozen and changes to be implemented without generator support, but doesn't introduce nearly as many changes. Based on feedback from @nirinchev
9931ebc
to
2bf0a3b
Compare
Future<dynamic> generatorTestBuilder(String directoryName, String inputFileName) async { | ||
final inputPath = _path.join(directoryName, inputFileName); | ||
final expectedPath = _path.setExtension(inputPath, '.expected'); | ||
return testBuilder( |
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 change breaks the idea that not all the input files could have a relevant expected result. It is possible the .expected
file to be missing, because of some other logic for expecting exception or expecting no output. Why is it changed?
if (d.operation < 0) { | ||
pen.red(bg: true); // delete | ||
} else if (d.operation == 0) { | ||
pen.reset(); // no-edit | ||
} else if (d.operation > 0) { | ||
pen.green(bg: true); // insert | ||
} |
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.
if (d.operation < 0) { | |
pen.red(bg: true); // delete | |
} else if (d.operation == 0) { | |
pen.reset(); // no-edit | |
} else if (d.operation > 0) { | |
pen.green(bg: true); // insert | |
} | |
switch (d.operation) { | |
case DIFF_DELETE: | |
pen.red(bg: true); | |
break; | |
case DIFF_EQUAL: | |
pen.reset(); | |
break; | |
case DIFF_INSERT: | |
pen.green(bg: true); | |
break; | |
default: | |
} |
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.
Another set of small suggestions
@@ -2,6 +2,9 @@ | |||
|
|||
**This project is in the Beta stage. The API should be quite stable, but occasional breaking changes may be made.** | |||
|
|||
### Breaking Changes | |||
* Use const constructed schema objects. ([#903](https://github.com/realm/realm-dart/pull/903)) |
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.
Is this a breaking change? I guess people just need to re-run the generator?
final objectType = property.ref.link_target.cast<Utf8>().toRealmDartString(treatEmptyAsNull: true); | ||
result &= schemaProperty.linkTarget == objectType; | ||
final isNullable = property.ref.flags & realm_property_flags.RLM_PROPERTY_NULLABLE != 0; | ||
result &= !(isNullable ^ schemaProperty.optional); // isNullable <=> schemaProperty.optional |
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.
That seems like an overly complicated way to express isNullable == schemaProperty.optional
.
@@ -2529,11 +2543,14 @@ extension on List<UserState> { | |||
extension on realm_property_info { | |||
SchemaProperty toSchemaProperty() { | |||
final linkTarget = link_target == nullptr ? null : link_target.cast<Utf8>().toDartString(); |
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.
final linkTarget = link_target == nullptr ? null : link_target.cast<Utf8>().toDartString(); | |
final linkTarget = link_target.cast<Utf8>().toRealmDartString(treatEmptyAsNull: true); |
propertyType: RealmPropertyType.values[type], | ||
optional: flags & realm_property_flags.RLM_PROPERTY_NULLABLE == realm_property_flags.RLM_PROPERTY_NULLABLE, | ||
primaryKey: flags & realm_property_flags.RLM_PROPERTY_PRIMARY_KEY == realm_property_flags.RLM_PROPERTY_PRIMARY_KEY, | ||
linkTarget: linkTarget == null || linkTarget.isEmpty ? null : linkTarget, |
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.
linkTarget: linkTarget == null || linkTarget.isEmpty ? null : linkTarget, | |
linkTarget: linkTarget, |
Postponed once more.. |
Const construct schema compile time
Dart has the ability to construct complex const objects compile time, which avoids any runtime overhead with regards to construction, and are never considered for garbage collection.
This is a very powerful feature, and it is used in Flutter to construct static widgets.
This PR explores the possibility of doing the same for our schema.
It introduces fly-weight property classes that are constructed compile time per realm model, and combined in a const constructed schema object.
These fly-weights are specialized by property type (ie. value, object, or list - set and map will come later), and help us deconstruct the static type of the collection, to get the static type of its elements.
This is useful to push type info and safety much deeper into the realm SDK, and allows us to avoid a fair amount runtime branching when reading from a realm. (The current PR doesn't do the same for writing yet)
It also refactor how we handle dynamic properties, which are now all accessed by
get<T>
including for lists, ie.get<List<T>>
. The error messages are also improved.The original idea came about, when implementing support for collections of nullable primitives, but was spun out into its own PR, as it is a significant change on its own.
The class hierarchy for realm objects changes slightly with this PR. It is now:
Here is an example of how the generated code looks like after this refactoring.
Given:
the generator produces:
Notice how the schema is now static const, as it is assembled from the likewise static const property fly-weights.