Skip to content

Conversation

technoir42
Copy link
Contributor

Depends on #625, permissions-dispatcher/kompile-testing#13
Will add some tests later.

@hotchemi
Copy link
Member

👀

@technoir42 technoir42 marked this pull request as ready for review June 17, 2019 16:58
Copy link
Member

@hotchemi hotchemi left a comment

Choose a reason for hiding this comment

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

Thank you! Commented a few but basically LGTM:D
I'd like to test a bit carefully before release.


private fun createWithPermissionCheckFun(rpe: RuntimePermissionsElement, method: ExecutableElement): FunSpec {
val builder = FunSpec.builder(withPermissionCheckMethodName(method))
.addOriginatingElement(rpe.element)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add an original element for every spec builder?👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might work if we set an origin element for just one spec that's guaranteed to be present in the file. But I think it's better to be safe and explicit and set origin element for every top-level spec we produce.
KotlinPoet 1.3.0+ deduplicates origin elements before passing them to Filer so kapt will see only one origin element.

// FIXME: weirdly under kaptKotlin files is not recognized as source file on AS or IntelliJ
// so as a workaround we generate .kt file in generated/source/kapt/$sourceSetName
// ref: https://github.com/hotchemi/PermissionsDispatcher/issues/320#issuecomment-316175775
val kaptGeneratedDirPath = processingEnv.options[KAPT_KOTLIN_GENERATED_OPTION_NAME]?.replace("kaptKotlin", "kapt")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure the issue has been already resolved or not👀
https://youtrack.jetbrains.com/issue/KT-20269

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was fixed.

Copy link
Member

@hotchemi hotchemi left a comment

Choose a reason for hiding this comment

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

Thank you! I'll test on my local and gonna release new ver soon😄

@hotchemi hotchemi merged commit 37b3257 into permissions-dispatcher:master Jun 18, 2019
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