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

Implement @DocumentId annotation #464

Merged
17 commits merged into from
Jun 4, 2019
Merged

Implement @DocumentId annotation #464

17 commits merged into from
Jun 4, 2019

Conversation

ghost
Copy link

@ghost ghost commented May 27, 2019

Implements @documentid support for deserialization of firestore document to custom POJO.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@ghost ghost force-pushed the ImplementDocumentIdAnnotation branch from 324a1f7 to 5a86518 Compare May 27, 2019 13:58
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Override cla and removed cla: no labels May 27, 2019
@ghost ghost requested a review from mikelehen May 27, 2019 14:29
ghost
ghost commented May 27, 2019
View reviewed changes
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Please review the implementation of DocumentId!

@ghost ghost marked this pull request as ready for review May 27, 2019 14:33
@ghost ghost assigned mikelehen May 27, 2019
mikelehen
mikelehen suggested changes May 31, 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.

This mostly looks very good (and thanks for the thorough tests!). I have a few small comments, and I think your refactor may be a behavior change, but I could also very easily be confused (I'm not very familiar with this code, so I was trying to come up to speed on it as I reviewed it).

Thanks for tackling this! Nice job working through this code.

firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java Outdated Show resolved Hide resolved
@mikelehen
Copy link
Contributor

Oh, also, I'm very sorry for the super slow review. With the offsite and everything, this took a lot longer than I would have liked. :)

@mikelehen mikelehen assigned ghost and unassigned mikelehen May 31, 2019
ghost
ghost commented Jun 3, 2019
View reviewed changes
Copy link
Author

@ghost ghost 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 thorough review!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes Override cla labels Jun 3, 2019
@ghost ghost assigned mikelehen and unassigned ghost Jun 3, 2019
@ghost ghost force-pushed the ImplementDocumentIdAnnotation branch from d013643 to 554ede5 Compare June 3, 2019 15:36
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Override cla and removed cla: no labels Jun 3, 2019
mikelehen
mikelehen approved these changes Jun 3, 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.

Two tiny error message nits (sorry!) but otherwise looks great. Thanks!

firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned ghost and unassigned mikelehen Jun 3, 2019
@ghost ghost merged commit 06c39a4 into master Jun 4, 2019
@rlazo rlazo deleted the ImplementDocumentIdAnnotation branch September 18, 2019 18:02
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Override cla size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@googlebot @mikelehen @google-oss-bot @wu-hui