-
Notifications
You must be signed in to change notification settings - Fork 578
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
Handle bridge methods while mapping #5626
Handle bridge methods while mapping #5626
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
fixes #5621 |
Thanks for the submission. We're taking a look. This one may require some additional tests to be added. |
@MarkDuckworth thank you! I've added some fixes |
Summary of this PR:
|
412f977
to
13f0dc5
Compare
13f0dc5
to
bd3bbca
Compare
Hi @vladd-g , Thank you again for your contribution again! Sorry about the late review. I will take a look at this PR in the coming weeks. |
Hi @cherylEnkidu, Thank you! For your convenience, I've left some notes here. If any questions arise during the review, please feel free to ask. |
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.
Hi @vladd-g ,
Thank you again for this contribution! It looks really good! Just one question before I commit any syntax changes to your work.
...se/src/main/java/com/google/firebase/database/core/utilities/encoding/CustomClassMapper.java
Outdated
Show resolved
Hide resolved
Hi @cherylEnkidu, Happy to hear that! Yes, absolutely. |
8478879
to
4c7c44d
Compare
f2aae6f
to
ef465a3
Compare
ef465a3
to
e1483e3
Compare
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.
The team was going through tasks shuffling last month. Sorry for the late review and thank you for the contribution again!
I understand the delay, that's completely fine. Thank you for the review and for merging the PR! |
This reverts commit 5ef5cdd.
This PR was inspired by [this question](https://stackoverflow.com/questions/77739613/kotlin-multiple-setters-getters-generics/). The `@Exclude` annotation has not been propagated to Kotlin's corresponding bridge methods. Additionally, there is no straightforward way to eliminate their 'get-' and 'set-' prefixes, as `@JvmName` is not propagated to them either. It seemed possible to solve this issue by redesigning the models. However, including bridge methods in the exceptions list – alongside other checks nearby – anyway seems to be a viable solution. --------- Co-authored-by: cherylEnkidu <[email protected]>
Restore PR: #5626 The majority of the code in this PR was contributed by [vladd-g](https://github.com/vladd-g) For more context, please refer to the linked PR.
Restore PR: #5626 The majority of the code in this PR was contributed by [vladd-g](https://github.com/vladd-g) For more context, please refer to the linked PR.
This PR was inspired by this question. The
@Exclude
annotation has not been propagated to Kotlin's corresponding bridge methods. Additionally, there is no straightforward way to eliminate their 'get-' and 'set-' prefixes, as@JvmName
is not propagated to them either.It seemed possible to solve this issue by redesigning the models. However, including bridge methods in the exceptions list – alongside other checks nearby – anyway seems to be a viable solution.