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.

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

Fallback to kotlin.Function + noinline #501

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

ShaishavGandhi
Copy link
Contributor

Initial pass for fixing issue in #500 .

I would love to add some tests for this, but I couldn't find any tests for Kotlin extension that are generated.

#500 has relevant details on the exact thing causing the error.

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

@elihart elihart 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 putting this up so quickly! it looks right, unfortunatley our testing tools don't support kotlin generated code, only java, so I don't have tests for this.

what I do though is add a class to the kotlinsample module and our checks make sure that that compiles

paramName,
type.toKPoet(nullable),
*modifiers.toKModifier().toTypedArray()
).build()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

use apply { } instead?

@@ -78,6 +79,10 @@ private fun JavaClassName.getPackageNameInKotlin(): String {
return packageName()
}

fun isLambda(type: JavaTypeName): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems that this should check the kotlin package too, since any other random class could technically contain this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check to see if it also contains the keyword kotlin.

"Collection",
"List",
"Map",
"Set",
"Iterable" -> kotlinCollectionsPkg
"String" -> kotlinPkg
"String", "Function1" -> kotlinPkg
Copy link
Contributor

Choose a reason for hiding this comment

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

is it necessary to check Function1 since the isLambda check is already done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops that's my bad. Leftover code from when i was experimenting.

@ShaishavGandhi
Copy link
Contributor Author

Thanks! Will add this sample to kotlinsample module.

I've struggled with Kotlin code generation testing also since we don't have an equivalent to the Google compile testing library for Kotlin.

I usually fallback to adding sample classes in the test directory of the sample and make sure it compiles. I think adding it in the sample will eventually muddy the module for people who're only looking for samples. By adding in the test directory, we can be more carefree with the implementation details of the models (and lazy 😄 ).

What do you think?

@elihart
Copy link
Contributor

elihart commented Aug 21, 2018

@shaishavgandhi05 that makes sense, the test directory would be great. Thanks a lot!

Hopefully we will have a kotlin compile testing library someday :P

* Add test folder in kotlinsample

Signed-off-by: shaishavgandhi05 <shaishgandhi@gmail.com>
@ShaishavGandhi
Copy link
Contributor Author

Anything blocking here? Have addressed the PR comments.

@elihart
Copy link
Contributor

elihart commented Aug 23, 2018

Nothing blocking, looks great, thanks!

@elihart elihart merged commit 18d5cbd into airbnb:master Aug 23, 2018
@ShaishavGandhi ShaishavGandhi deleted the sg/kotlin-extension-fix branch August 23, 2018 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants