realm / realm-dart

Realm is a mobile database: a replacement for SQLite & ORMs.
Apache License 2.0
773 stars 86 forks source link

RDART-969: Keypath filtering on collections #1714

Closed papafe closed 5 months ago

papafe commented 5 months ago

This PR adds support for keypath filtering on collections

TODO:

papafe commented 5 months ago

@nielsenko I've put the bulk of the tests for the keypath filtering on results, and I've left some basic tests for the other collections (lists, sets, dictionaries), so we don't repeat the same tests. I have left some todos around about things I am not sure / we need to discuss, but functionally everything should work.

coveralls-official[bot] commented 5 months ago

Pull Request Test Coverage Report for Build 9477711446

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/realm_dart/lib/src/list.dart 12 13 92.31%
packages/realm_dart/lib/src/map.dart 13 14 92.86%
packages/realm_dart/lib/src/set.dart 12 13 92.31%
<!-- Total: 87 90 96.67% -->
Totals Coverage Status
Change from base Build 9416962455: 0.2%
Covered Lines: 5996
Relevant Lines: 6895

💛 - Coveralls
papafe commented 5 months ago

@nielsenko I think I've addressed all the comments. A couple of other things I've noticed, and that we don't necessarily need to address here, but I thought it would be worth writing down.

We have some inconsistencies with naming:

  1. In the errors for unmanaged sets we call them RealmSets, while we call lists just Lists (same for maps)
  2. Regarding notification controllers, we have ListNotificationController but RealmSetNotificationController.
  3. Regarding tests, we have list_test but we have realm_set_test/realm_map_test
  4. Maybe we should have a different suffix between the native handle implementation and the interface files. just having a look is a little bit confusing sometimes.

Some other considerations (that probably require some work):

  1. Maybe we can move the keypath property to the base NotificationController, instead of having it defined in the various specific classes
  2. Maybe we can have a base class for all collections (results, lists, sets, maps). This way we could move changes and some other common things there. The collection handle themselves could have a common base class.
nielsenko commented 5 months ago

In my opinion..

  1. In the errors for unmanaged sets we call them RealmSets, while we call lists just Lists (same for maps)

Going forward we should try to reduce the use of realm prefix to where it is needed. This means only on public facing classes, that are likely to collide with other class names (RealmSet, RealmList, RealmMap, etc.). Internally we should avoid unless it makes code more readable.

  1. Regarding notification controllers, we have ListNotificationController but RealmSetNotificationController.

It should just be SetNotificationController.

  1. Regarding tests, we have list_test but we have realm_set_test/realm_map_test

it should just be set_test.dart.

  1. Maybe we should have a different suffix between the native handle implementation and the interface files. just having a look is a little bit confusing sometimes.

I'm a bit reluctant on this. For each handle, there is the interface, and two implementations.. I would prefer not to suffix the implementations with Native and Web (or whatever)

Some other considerations (that probably require some work):

  1. Maybe we can move the keypath property to the base NotificationController, instead of having it defined in the various specific classes

  2. Maybe we can have a base class for all collections (results, lists, sets, maps). This way we could move changes and some other common things there. The collection handle themselves could have a common base class.

The current C-api don't really give us much benefit, but yes.

However, let us land this PR as is, and get started on the weekend 😄 .. we can always address this later.

papafe commented 5 months ago

I'll merge this as soon as the CI agrees