Fix order by multiple field with different orders and added Filter support. #319
Conversation
ominibyte
force-pushed
the
fix-multiple-orderby
branch
from
52262ad to
64dc22b
Compare
This PR addresses the following: 1. Fixes an issue when ordering by multiple fields with different sort orders 2. Closes atn832#293 with support for Filter.or and Filter.and
ominibyte
force-pushed
the
fix-multiple-orderby
branch
from
64dc22b to
8b3ede9
Compare
ominibyte
changed the title
|
Wow thank you for tackling these big issues! It's a shame you didn't split it into two PRs. It'll take me quite a bit longer to review. Would you mind splitting them if it is not too much to ask? If the issues are tightly coupled, don't worry about it and I'll review this PR in a week or two. |
| } | ||
|
|
||
| sortedList.sort((d1, d2) { | ||
| var compare = doCompare(fields.first, directions.first, d1, d2); |
There was a problem hiding this comment.
I guess it's just a matter of preference. This could work too and is sligthly more readable:
for (var i = 0; i < fields.length; i++) {
final compare = doCompare(fields[i], directions[i], d1, d2);
if (compare != 0) {
return compare;
}
}
return 0;There was a problem hiding this comment.
I ended up making the change at 57311c7.
| whereNotIn: whereNotIn, | ||
| isNull: isNull); | ||
| }).toList(); | ||
| if (field is Filter) { |
There was a problem hiding this comment.
At first glance, it is a bit difficult to see that the only thing that changes is the predicate. We could rewrite it like so:
final predicate = field is Filter ?
_buildFilterPredicate(field.toJson())
: (document) => _wherePredicate(document, field,
isEqualTo: isEqualTo,
isNotEqualTo: isNotEqualTo,
isLessThan: isLessThan,
isLessThanOrEqualTo: isLessThanOrEqualTo,
isGreaterThan: isGreaterThan,
isGreaterThanOrEqualTo: isGreaterThanOrEqualTo,
arrayContains: arrayContains,
arrayContainsAny: arrayContainsAny,
whereIn: whereIn,
whereNotIn: whereNotIn,
isNull: isNull)
final operation = (List<DocumentSnapshot<T>> docs) => docs
.where(predicate)
.toList();
return MockQuery<T>(this, operation);| return MockQuery<T>(this, operation); | ||
| } | ||
|
|
||
| bool Function(DocumentSnapshot<T> document) _buildFilterPredicate( |
There was a problem hiding this comment.
Perhaps explaining briefly in the method's docs how a Filter gets serialized to JSON could help others understand the code faster.
There was a problem hiding this comment.
GitHub doesn't really show the preexisting tests above. So it is not immediately clear which tests were simply a copy/paste of existing tests with Filter applied, and which ones are novel. Pointing out the few ones that are novel would have helped me review.
| @@ -1269,6 +1705,64 @@ void main() { | |||
| ]); | |||
| }); | |||
|
|
|||
| test('orderBy with second field descending', () async { | |||
| .snapshots() | ||
| .listen(expectAsync1((QuerySnapshot snapshot) { | ||
| expect(snapshot.docs.length, equals(1)); | ||
| expect(snapshot.docs.first.get('name'), equals('Washington')); |
|
I just published your code to https://pub.dev/packages/fake_cloud_firestore/changelog#310 and updated the README with the new features you implemented. Thanks again! |
|
Thanks, I saw the changes you made and the published version. |
This PR addresses the following: