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

Add IN and ARRAY_CONTAINS_ANY (not publicly exposed) #519

Merged
merged 18 commits into from
Jun 20, 2019
Merged

Add IN and ARRAY_CONTAINS_ANY (not publicly exposed) #519

merged 18 commits into from
Jun 20, 2019

Conversation

thebrianchen
Copy link

Porting over from Web SDK.

Pending adding support to run against Firestore Emulator.

@googlebot googlebot added the cla: yes Override cla label Jun 13, 2019
mikelehen
mikelehen suggested changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

I have a few porting quibbles / nits, but mostly this looks great!

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/RelationFilter.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 14, 2019
thebrianchen
thebrianchen commented Jun 19, 2019
View reviewed changes
Copy link
Author

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

Updated this to not use emulator tests so that Gil can port firebase/firebase-js-sdk#1894 on top of this.

@thebrianchen thebrianchen removed their assignment Jun 19, 2019
rockwotj
rockwotj approved these changes Jun 19, 2019
View reviewed changes
Copy link

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

This LGTM, have some more NaN edge cases to test :)

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Show resolved Hide resolved
mikelehen
mikelehen suggested changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits...

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen and rockwotj Jun 19, 2019
thebrianchen
thebrianchen commented Jun 20, 2019
View reviewed changes
Copy link
Author

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

Thanks for the review and catching the typos -- I'll be sure to be more thorough next time.

@thebrianchen thebrianchen removed the request for review from wilhuff June 20, 2019 00:45
@thebrianchen thebrianchen removed their assignment Jun 20, 2019
mikelehen
mikelehen suggested changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for cleaning up the serializer tests.

This LGTM but we can't check this in with the API publicly exposed, so if the plan is to check this in (which seems like a good idea!) then we'll need to hide them.

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 20, 2019
@mikelehen mikelehen assigned thebrianchen and unassigned mikelehen Jun 20, 2019
@thebrianchen thebrianchen merged commit 745272a into master Jun 20, 2019
@thebrianchen thebrianchen mentioned this pull request Jun 24, 2019
Merged
schmidt-sebastian pushed a commit that referenced this pull request Jun 25, 2019
@schmidt-sebastian
Add IN and ARRAY_CONTAINS_ANY (not publicly exposed) (#519)
100b904
schmidt-sebastian pushed a commit that referenced this pull request Jun 27, 2019
@schmidt-sebastian
Add IN and ARRAY_CONTAINS_ANY (not publicly exposed) (#519)
d93feaf
@thebrianchen thebrianchen mentioned this pull request Aug 2, 2019
Merged
@rlazo rlazo deleted the bc/in-client branch September 27, 2019 14:53
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mikelehen mikelehen mikelehen approved these changes

@rockwotj rockwotj rockwotj approved these changes

Assignees

@thebrianchen thebrianchen

Labels
api: firestore cla: yes Override cla size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thebrianchen @mikelehen @rockwotj @googlebot @google-oss-bot