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

Reflect the annotations declared in constructor params #519

Merged
merged 4 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@shaishavgandhi05
Contributor

shaishavgandhi05 commented Sep 2, 2018

If I declare a model like:

abstract class AnnotationModel(@StringRes val resId: Int): EpoxyModelWithHolder<AnnotationHolder>() {}

the Kotlin extension generated doesn't add the annotation to the extension function. However, it is added to the AnnotationModel_ class.

This PR fixes that.

shaishavgandhi05 added some commits Sep 2, 2018

Reflect the annotations declared in constructor params
Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
Add converter as KPoet Extension
Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
@elihart

@shaishavgandhi05 thanks for this! Just one request - looks good besides that

Add tests for PoetExtensions
Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
@shaishavgandhi05

This comment has been minimized.

Show comment
Hide comment
@shaishavgandhi05

shaishavgandhi05 Sep 5, 2018

Contributor

Have updated the PR with a bunch of tests for PoetExtensions. Also, as noted in the comment before, some investigation revealed that it was hard to transfer parameters from JavaAnnotationSpec to KotlinAnnotationSpec since JavaCodeBlock has no accessors that will let me move it to KotlinCodeBlock.

Contributor

shaishavgandhi05 commented Sep 5, 2018

Have updated the PR with a bunch of tests for PoetExtensions. Also, as noted in the comment before, some investigation revealed that it was hard to transfer parameters from JavaAnnotationSpec to KotlinAnnotationSpec since JavaCodeBlock has no accessors that will let me move it to KotlinCodeBlock.

@elihart

@shaishavgandhi05 hm, that param limitation is understandable so it seems fine to not support for now. Thank you for adding the comment and tests though.

I just have one last concern, I don't want code breaking if people do use annotations with values. For example, annotations like FloatRange are not that rare in android, and if generated code loses the parameters it won't compile.

Could we check which annotations have parameters and just omit those for now?

I can ship a new release as soon as this is merged :)

import org.junit.Test
import javax.lang.model.element.Modifier
class PoetExtensionsTest {

This comment has been minimized.

@elihart

elihart Sep 5, 2018

Contributor

Thanks a lot for the tests!

@elihart

elihart Sep 5, 2018

Contributor

Thanks a lot for the tests!

Show outdated Hide outdated epoxy-processor/src/main/java/com/airbnb/epoxy/PoetExtensions.kt
Make KotlinAnnotationSpec nullable
* Add test asserting null behavior

Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
@shaishavgandhi05

This comment has been minimized.

Show comment
Hide comment
@shaishavgandhi05

shaishavgandhi05 Sep 5, 2018

Contributor

Yup you're right. Don't want to break existing functionality. I've added a test for the null case as well as added it to the sample model so that we are sure it doesn't cause any compile errors.

Contributor

shaishavgandhi05 commented Sep 5, 2018

Yup you're right. Don't want to break existing functionality. I've added a test for the null case as well as added it to the sample model so that we are sure it doesn't cause any compile errors.

@elihart

This comment has been minimized.

Show comment
Hide comment
@elihart

elihart Sep 5, 2018

Contributor

Excellent, thanks again! I'll try to push a new release asap

Contributor

elihart commented Sep 5, 2018

Excellent, thanks again! I'll try to push a new release asap

@elihart elihart merged commit 2712b45 into airbnb:master Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shaishavgandhi05 shaishavgandhi05 deleted the shaishavgandhi05:sg/reflect-constructor-annotations branch Sep 5, 2018

@shaishavgandhi05

This comment has been minimized.

Show comment
Hide comment
@shaishavgandhi05

shaishavgandhi05 Sep 5, 2018

Contributor

No hurries. It's a nice to have, not a deal breaker 😄

Contributor

shaishavgandhi05 commented Sep 5, 2018

No hurries. It's a nice to have, not a deal breaker 😄

@elihart elihart referenced this pull request Sep 6, 2018

Merged

Prepare for release 2.17.0 #529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment