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

Annotation processors in Kotlin #775

Merged
merged 14 commits into from
Apr 3, 2017
Merged

Annotation processors in Kotlin #775

merged 14 commits into from
Apr 3, 2017

Conversation

geralt-encore
Copy link
Collaborator

@geralt-encore geralt-encore commented Mar 30, 2017

Since it is possible to use Kotlin in annotation processing without adding it as a dependency to the library itself I thought that it makes sense to use the best available tool (and Kotlin is for sure better than Java). I hope you won't mind this change.
Unfortunately, not everything is smooth. I encountered into two issues so far:

  • AutoService doesn't work with Kotlin classes so as a workaround I extended actual processors with dummy Java classes and annotated them.

  • Another problem, which I haven't solved is about running tests for processors and building sample apps from IDE. It works from a console but fails with whole code full of unresolved references from IntelliJ. It would be really nice if someone could take a look at it since I haven't figured out how to fix it.

Besides these issues everything works and looks nicer than it was before in Java :D

@codecov-io
Copy link

codecov-io commented Mar 30, 2017

Codecov Report

Merging #775 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #775   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          87       87           
  Lines        2638     2638           
  Branches      296      296           
=======================================
  Hits         2555     2555           
  Misses         50       50           
  Partials       33       33

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8575ad9...b235051. Read the comment docs.

@geralt-encore
Copy link
Collaborator Author

geralt-encore commented Mar 31, 2017

Just discovered that everything works from IDE on my work laptop. Just need to figure out what is the difference between setups.

@geralt-encore
Copy link
Collaborator Author

Yep, the issue was in my weird Gradle configuration for Android Studio. So everything is working without any issues.

Copy link
Collaborator

@nikitin-da nikitin-da left a comment

Choose a reason for hiding this comment

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

Hoho looks nice! 👍 👍
just few nits

build.gradle Outdated
@@ -50,6 +54,7 @@ ext.libraries = [
// Core libraries
supportAnnotations : 'com.android.support:support-annotations:' + supportLibsVersion,
rxJava : 'io.reactivex:rxjava:1.2.1',
kotlinStdLib : "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version",
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting)

build.gradle Outdated
@@ -67,6 +72,7 @@ ext.libraries = [
assertJ : 'org.assertj:assertj-core:3.6.2',
assertJAndroid : 'com.squareup.assertj:assertj-android:1.1.1',
mockitoCore : 'org.mockito:mockito-core:2.7.19',
mockitoKotlin : 'com.nhaarman:mockito-kotlin:1.3.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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


/**
* Base annotation processor for StorIO.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

without double empty line?

*
* It will be called after Java Compiler will find lang elements annotated with annotations from [getSupportedAnnotationTypes].
* @param annotations set of annotations
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

el here and below

}


Copy link
Collaborator

Choose a reason for hiding this comment

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

EL


return StorIOContentResolverTypeMeta(simpleName, packageName, storIOContentResolverType)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

EL

.addAnnotation(ANDROID_NON_NULL_ANNOTATION_CLASS_NAME)
.build())
.addCode("""return DeleteQuery.builder()
$INDENT.uri(${"$"}S)
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting query builders here and below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this formatting bothers me as well. The thing is that I don't know how to format it since it is a literal string which sounds like a good idea for this case, but this formatting...
I can change it to concatenation if you think that is' better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep I agree that with concatenation it will not look pretty too =(
Up to you

Copy link
Member

Choose a reason for hiding this comment

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

You can add indentation and then apply trimIndent()


val superclass = ClassName.get("com.pushtorefresh.storio.contentresolver", SUFFIX)
val superclassParametrized = ParameterizedTypeName.get(superclass, storIOSQLiteTypeClassName)

Copy link
Collaborator

Choose a reason for hiding this comment

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

EL

/**
* Annotation processor for StorIOSQLite.
*
* It'll process annotations to generate StorIOSQLite
Copy link
Collaborator

Choose a reason for hiding this comment

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

place Object-Mapping on the same line


val superclass = ClassName.get("com.pushtorefresh.storio.sqlite", SUFFIX)
val superclassParametrized = ParameterizedTypeName.get(superclass, typeClassName)

Copy link
Collaborator

Choose a reason for hiding this comment

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

EL

@artem-zinnatullin
Copy link
Member

screen shot 2017-04-03 at 00 56 46

niice

@nikitin-da
Copy link
Collaborator

@artem-zinnatullin PTAL please, because you have rich experience 🐕 🍲 🍴 in Kotlin


dependencies {
compile libraries.intellijAnnotations
compile libraries.autoService
compile libraries.javaPoet
compile libraries.kotlinStdLib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it increase module methods count on 6k? =(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember the exact number of methods in Kotlin stdlib, but it doesn't matter anyway because this dependency isn't going to be in apk so dex limit won't be affected.

Copy link
Member

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

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

Great! Few places that use forEach could be rewritten to kotlin streams api, but I'm ok with current variant :)

import javax.lang.model.type.TypeKind
import javax.lang.model.type.TypeMirror

class JavaTypeTest {
Copy link
Member

Choose a reason for hiding this comment

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

Next step would be converting JUnit tests to Spek :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I wanted to try it for quite some time, so I might take a look into it :)

Copy link
Member

Choose a reason for hiding this comment

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

There are few problems with it though, but I can help with that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I know about it. Tried to setup it once - spend about an hour without success and given up until the next try.

.addAnnotation(ANDROID_NON_NULL_ANNOTATION_CLASS_NAME)
.build())
.addCode("""return DeleteQuery.builder()
$INDENT.uri(${"$"}S)
Copy link
Member

Choose a reason for hiding this comment

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

You can add indentation and then apply trimIndent()

@geralt-encore
Copy link
Collaborator Author

@artem-zinnatullin thanks for pointing out to trimIndent() I didn't know about it. Will make a fix right away. Could you elaborate more about Kotlin streams api? Didn't quite get what do you mean.

@artem-zinnatullin
Copy link
Member

Usually instead of let's call it "imperative" iterating over collections and populating some mutable data structures you can use more "functional" approach with operators like map/flatMap and so on in order to transform one collection into another, I can't say that it definitely worth it in your cases in this PR because there are pretty complicated and optimized for performance, but usually it's possible and makes you rethink the imperatively written code into more separated pieces of data transformation

@geralt-encore
Copy link
Collaborator Author

Ah, I thought that you were talking about something different. Of course, I know about functional operators like map/flatMap. I think that I used forEach just for iteration cases since there weren't actual transformations.

@geralt-encore geralt-encore merged commit da7a1f5 into pushtorefresh:master Apr 3, 2017
@geralt-encore geralt-encore deleted the kotlin branch April 3, 2017 20:53
@artem-zinnatullin
Copy link
Member

Well, they are, but that's fine :)

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.

4 participants