Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor RealmMetadata and remove TableHandle #2230

Merged
merged 10 commits into from
Feb 17, 2021
Merged

Refactor RealmMetadata and remove TableHandle #2230

merged 10 commits into from
Feb 17, 2021

Conversation

papafe
Copy link
Contributor

@papafe papafe commented Feb 5, 2021

This PR refactors RealmMetadata to create a correspondence between the RealmObject.Metadata and the TableKey. Given that with the TableKey we can get a reference to the table in native, TableHandle has been removed too.

Fixes #2213

@papafe papafe marked this pull request as draft February 5, 2021 09:16
nirinchev
nirinchev reviewed Feb 5, 2021
View reviewed changes
Realm/Realm/Handles/ObjectHandle.cs Outdated Show resolved Hide resolved
Realm/Realm/Handles/SharedRealmHandle.cs Outdated Show resolved Hide resolved
Realm/Realm/Handles/SharedRealmHandle.cs Outdated Show resolved Hide resolved
Realm/Realm/Native/TableKey.cs Outdated Show resolved Hide resolved
Realm/Realm/Realm.cs Show resolved Hide resolved
wrappers/src/shared_realm_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/shared_realm_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/shared_realm_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/shared_realm_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/table_cs.cpp Show resolved Hide resolved
@papafe papafe marked this pull request as ready for review February 9, 2021 14:04
nirinchev
nirinchev reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor simplification suggestions from me, otherwise should be good to go once the Core PR is merged.

Realm/Realm/Native/TableKey.cs Outdated Show resolved Hide resolved
Realm/Realm/Native/TableKey.cs Outdated Show resolved Hide resolved
Realm/Realm/Realm.cs Outdated Show resolved Hide resolved
wrappers/src/object_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/shared_realm_cs.cpp Outdated Show resolved Hide resolved
wrappers/src/sort_descriptor_cs.cpp Outdated Show resolved Hide resolved
nirinchev
nirinchev approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but probably should be rebased on top of master to get the build fixed.

Realm/Realm/Native/TableKey.cs Outdated Show resolved Hide resolved
@papafe papafe force-pushed the fp/realm-metadata-2213 branch from a18fa20 to 668593c Compare February 11, 2021 09:26
@papafe papafe force-pushed the fp/realm-metadata-2213 branch from 668593c to 6ec51e3 Compare February 11, 2021 10:05
@papafe papafe requested a review from LaPeste February 11, 2021 12:47
LaPeste
LaPeste reviewed Feb 12, 2021
View reviewed changes
wrappers/src/shared_realm_cs.cpp Outdated
const ObjectSchema& object_schema = *realm->schema().find(object_name);

const TableRef table = get_table(realm, table_key);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all these new lines needed?

LaPeste
LaPeste suggested changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@LaPeste LaPeste left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the 2 minuscule comments I had, one more. ObjectKey is renamed to TableKey (plus other changes) and you changed the year on the copyright. I'm not sure what the policy is but I assume all the other files you modified need to be changed, as well? If not, what's the policy about copyrights?

Anyway, beside the very small things mentioned above, PR looks good to me.

@papafe
Copy link
Contributor Author

papafe commented Feb 12, 2021

@LaPeste agree on both points. Regarding ObjectKey renamed to TableKey... it's just git that thinks that I renamed it. I actually deleted ObjectKey and added TableKey, but because they are similar git thinks I have renamed it, that's why the copyright difference.

@papafe
Copy link
Contributor Author

papafe commented Feb 12, 2021

This PR is considered mergeable, once the core changes have been approved (realm/realm-core#4413) and merged to the v11 branch

@nirinchev
Copy link
Member

The test failures are caused by a change in the Core query syntax. They'll be fixed once #2214 is merged.

@papafe papafe mentioned this pull request Feb 16, 2021
Merged
2 tasks
@nirinchev
Copy link
Member

Failing tests seem to be my fault - the AOT dynamic runtime doesn't seem to like my code.

@papafe papafe merged commit 1f96adc into master Feb 17, 2021
@papafe papafe deleted the fp/realm-metadata-2213 branch February 17, 2021 08:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nirinchev nirinchev nirinchev approved these changes

@LaPeste LaPeste LaPeste approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Realm Metadata
3 participants
@papafe @nirinchev @LaPeste