Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support RealmValue (aka mixed) #1051
Support RealmValue (aka mixed) #1051
Changes from 18 commits
570d453
1089abc
635d5b5
7863668
b162451
dfbf3b0
f49b875
f3ea39f
8101c46
2151339
6b3cd60
95204e2
abc10fb
c500274
a1463cf
b392671
502d5c2
0dcdb77
8530dc7
d2298ea
6ab7a1c
4d8bf20
634a8f1
9acd3a2
cd47f1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we be more descriptive with these error messages? In my experience people don't understand short errors like these and reach out to support to ask them what they mean. We could rephrase that to something like:
'wrong' is declared as 'RealmValue?' which is not allowed. 'RealmValue' can already represent null with 'RealmValue.nullValue()', so you don't need to declare the property itself as nullable.
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.
In general I agree that we should have long descriptive error messages, but we need to agree on the team. Previously I was asked to simplify 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.
I would vote for short and clear messages. We could reword this to be more clear, but long error messages stand in the way after a while. So imo, this could be
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 don't think it gets in the way. The short version is at the top, the rest you can read if you are confused.
But I don't think this is specific to this PR. If we want more verbose error messages, I think we should change a lot of 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.
I don't think this handles the case where you open a Realm with an incomplete schema that contains
RealmValue
properties linking to tables not in the user schema.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 will add a test for this case.
What should be the expected outcome? Do we error out, or use dynamic realm objects?
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.
We should return dynamic objects.
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.
Hmm .. added a test, but it crashes in
realmCore.getProperty(object, propertyMeta.key);
At least the c-api don't support this