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

Support Kotlin trailing lambda syntax for custom output formatters #690

Merged
merged 4 commits into from Sep 11, 2022

Conversation

vjgarciag96
Copy link
Collaborator

Fixes #584.

Support Kotlin trailing lambda syntax for custom output formatters

  • This is done by offering a new API in which an output formatter can be specified as org.gradle.api.Action. Gradle automatically maps Groovy closures and Kotlin lambdas to arguments of that type (https://docs.gradle.org/current/userguide/kotlin_dsl.html#groovy_closures_from_kotlin).

  • Internally, there have been some changes done to support this:

    • The type of the property that is used to set an output formatter is Any, and this isn't the nicer because tooling can't really hint users of the API with the correct types, and we also have to deal manually with the types of input.
    • The new type we wanted to support (Action) would run into type erasure problems if we used this existing property and we would need to have code casting the types in an unsafe way (as we can't know if a provided argument is Action or Action of something else).
    • To sort this out, we have kept the existing property for compatibility reasons (it can still be used to set String, Reporter or Closure output formatters) but we have added a new function to set Action output formatters. With this we achieve maintaining public API compatibility whilst adding support for the new Action output formatters with improved type support (internally, safe use of types, and externally, type hinting in tooling and clear public API signature).

Testing

Manual
Manually tested that we can now use the trailing lamdba syntax in Kotlin, and that everything works as before both in Groovy and Kotlin. Also verified that we get type hints in the IDE when using the new API:

image

Automated
Added a couple of tests covering the new changes.

* This is done by offering a new API in which an output formatter can be specified as org.gradle.api.Action<Result>. Gradle automatically maps Groovy closures and Kotlin lambdas to arguments of that type (https://docs.gradle.org/current/userguide/kotlin_dsl.html#groovy_closures_from_kotlin).

* Internally, there have been some changes done to support this:
  - The type of the property that is used to set an output formatter is `Any`, and this isn't the nicer because tooling can't really hint users of the API with the correct types, and we also have to deal manually with the types of input.
  - The new type we wanted to support (Action<Result>) would run into type erasure problems if we used this existing property and we would need to have code casting the types in an unsafe way (as we can't know if a provided argument is Action<Result> or Action of something else).
  - To sort this out, we have kept the existing property for compatibility reasons (it can still be used to set String, Reporter or Closure output formatters) but we have added a new function to set Action<Result> output formatters. With this we achieve maintaining public API compatibility whilst adding support for the new Action<Result> output formatters with improved type support (internally, safe use of types, and externally, type hinting in tooling and clear public API signature).
@ben-manes
Copy link
Owner

Thanks!

I believe there was still some remaining problems to work out after the kotlin migration (#673), so we may not be in a stable state to be able to perform a release. That issue has an answer but I don't think it was followed up on.

Looking at the plugins that use ours, the gradle-libraries-plugin appears to call some the newly marked internal classes (@fkorotkov). It might be kinder to keep them public for the usages to adapt, but otherwise not try to maintain backwards compatibility.

The changes make sense and the code looks good to me. @jaredsburrows can you please review?

@vjgarciag96
Copy link
Collaborator Author

@ben-manes Thanks for feedback!

I believe there was still some remaining problems to work out after the kotlin migration (#673), so we may not be in a stable state to be able to perform a release. That issue has an answer but I don't think it was followed up on.

Ohhh I see, thanks for the heads-up! I might have a look into those other problems too then.

Looking at the plugins that use ours, the gradle-libraries-plugin appears to call some the newly marked internal classes (@fkorotkov). It might be kinder to keep them public for the usages to adapt, but otherwise not try to maintain backwards compatibility.

That makes sense to me, everything is back to public again!

@jaredsburrows
Copy link
Collaborator

I wonder if converting from Closure to Action would help resolve the problem we are seeing in #673.

@@ -12,6 +14,7 @@ import org.gradle.api.tasks.Internal
import org.gradle.api.tasks.Optional
import org.gradle.api.tasks.TaskAction
import org.gradle.util.ConfigureUtil
import java.lang.IllegalArgumentException
Copy link
Collaborator

Choose a reason for hiding this comment

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

This import should not be needed in Kotlin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right, removed!


companion object {
@JvmField
val DEFAULT = BuiltIn(formatterNames = "text")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this have to be a @JvmField?

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 added it to be able to access this from Groovy code in tests. But I have removed it now as it is not necessary in production code.

@vjgarciag96
Copy link
Collaborator Author

I wonder if converting from Closure to Action would help resolve the problem we are seeing in #673.

If the problem there has to do with rejectVersionIf, Action can't be used because it doesn't have a return type and rejectVersionIf's argument needs to return a boolen. But I think the suggestion from the comment should work (make the ComponentFilter interface functional). I can give it a go and put up a PR if it works.

Copy link
Owner

@ben-manes ben-manes 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 contributing! please merge when you’re ready.

@vjgarciag96 vjgarciag96 merged commit cc714f8 into ben-manes:master Sep 11, 2022
@jaredsburrows
Copy link
Collaborator

Thanks!

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.

Question: Setup outputFormatter using Closure in Kotlin
3 participants