-
Notifications
You must be signed in to change notification settings - Fork 64
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
Query support #81
Query support #81
Conversation
…nto cr/cinterop
…nto cr/cinterop
I looked into the links, dug a bit into the Java implementation and tried to play with a lazy evaluated API to get some insight. The last commit holds an overly lazy implementation, that raises question about how to tweak doing the fluent queries, when to raise errors, etc. Will try to do the basic blocks for Native and then we can maybe discuss the various choices in a meeting. |
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.
Nice stab at the query API, I only have one worry concerning throwing for an empty result as stated in the comments
packages/cinterop/src/androidTest/java/io/realm/interop/CinteropTest.kt
Outdated
Show resolved
Hide resolved
packages/cinterop/src/darwinCommon/kotlin/io/realm/interop/RealmInterop.kt
Outdated
Show resolved
Hide resolved
packages/cinterop/src/darwinCommon/kotlin/io/realm/interop/RealmInterop.kt
Outdated
Show resolved
Hide resolved
@@ -53,7 +53,7 @@ class RealmConfiguration private constructor( | |||
data class Builder( | |||
var path: String? = null, | |||
var name: String = "default", // Optional Realm name (default is 'default') | |||
var schema: Any, | |||
var schema: Any? = null, |
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.
for now, it's still required to provide a schema until #90 is fixed
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.
Requiring it here just ruins the ability to build the configuration fluently. As far as I remember there is already a check for the schema
when building it.
packages/library/src/commonMain/kotlin/io/realm/RealmResults.kt
Outdated
Show resolved
Hide resolved
@@ -52,6 +53,23 @@ class InstrumentedTests { | |||
sample.name = "Hello, World!" | |||
kotlin.test.assertEquals("Hello, World!", sample.name) | |||
realm.commitTransaction() | |||
|
|||
val objects1: RealmResults<Sample> = realm.objects(Sample::class) |
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 add the below to its own query test?
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 tried separating the tests a bit. The really annoying part is that we have to maintain it twice as we currently cannot trigger the common tests to run on Android (neither from UI) or by connectedAndroidTest
.
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.
Besides somehow overcoming the above annoyance, I guess reorganizing test should maybe follow the actual design/implementation. This was merely a PR to show a basic round trip to the underlying query API.
val objects2: RealmResults<Sample> = realm.objects(Sample::class).query("name == $0", "Hello, World!") | ||
val sample2 = objects2[0] | ||
val objects2: RealmResults<Sample> = | ||
realm.objects(Sample::class).query("name == $0", s) |
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.
Reminder to add a small synopsis of the query syntax (could be in another PR)
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 have put a very brief note, basically just pointing to JS. Think it makes most sense to start elaborating along with #84.
Initial implementation of query support
This mostly just implements the inner building blocks required to do actual queries, but is not trying to finalize the public query API. This task is tracked by #84
For now the query support is implemented as lazy as possible only triggering C-API calls when actually accessing objects. Inspired by JS/Swift SDK's, but not necessarily semantic equivalent, as maybe questions about building and executing queries are not settled yet.
Closes #64