No longer required to specify closureOf<Distribution> #144
Conversation
jmatsu
force-pushed
the
fix/avoid_specifying_type_of_distriubtion
branch
from
6e1bd9d to
26fab97
Compare
jmatsu
force-pushed
the
fix/avoid_specifying_type_of_distriubtion
branch
from
364e31c to
d8467c2
Compare
| - '3.5.1' | ||
| - '3.6.3' | ||
| - '4.1.0' | ||
| - '4.2.0' |
There was a problem hiding this comment.
The previous build couldn't handle multiple parameters for 4.2.0 properly. FYI, 4.2.0's parameters are already defined in include section and it's the correct way to add multiple parameters.
| @@ -0,0 +1,211 @@ | |||
| package com.deploygate.gradle.plugins.dsl; | |||
There was a problem hiding this comment.
We are gonna use Java 🤷
henteko
left a comment
henteko
left a comment
There was a problem hiding this comment.
I wrote two question, please check!
| result = 31 * result + (sourceFile != null ? sourceFile.hashCode() : 0); | ||
| result = 31 * result + (message != null ? message.hashCode() : 0); | ||
| result = 31 * result + (skipAssemble ? 1 : 0); | ||
| result = 31 * result + getDistribution().hashCode(); |
There was a problem hiding this comment.
| result = 31 * result + getDistribution().hashCode(); | |
| result = 31 * result + (hasDistribution() ? getDistribution().hashCode() : 0); |
Existing code returns 0 if distribution is null.
Is there any reason not to return 0?
There was a problem hiding this comment.
These code are auto-generated and following the best practice in JVM world. null should be zero, otherwise should have any value.
There was a problem hiding this comment.
I did not know this code was auto-generated.
Looks good!
|
|
||
| @Nonnull | ||
| public Distribution getDistribution() { | ||
| return optionalDistribution[0]; |
There was a problem hiding this comment.
https://github.com/DeployGate/gradle-deploygate-plugin/pull/144/files#diff-0ac52e1ba8973482bdd7896222e2135ae8c15a3b8336aadff46afc50683a3614L48
The existing code excludes empty.
The code always returns the 0th Distribution.
Is this a not problem in behavior?
There was a problem hiding this comment.
Totally fine. We can ignore a few rare cases because this field is only for internal use. And also, Groovy has optional chaining but Java not. That's why it's good to convert nullable to non-null.
|
@henteko Thank you for your reviews! Could you plz check my replies? |
| result = 31 * result + (sourceFile != null ? sourceFile.hashCode() : 0); | ||
| result = 31 * result + (message != null ? message.hashCode() : 0); | ||
| result = 31 * result + (skipAssemble ? 1 : 0); | ||
| result = 31 * result + getDistribution().hashCode(); |
There was a problem hiding this comment.
I did not know this code was auto-generated.
Looks good!
|
Thanks as always! |
For Kotlin DSL users,
closureOf<Distribution>seems to be redundant. To avoid this issue, Gradle recommends to provideorg.gradle.api.Actioninterfaces when writing Gradle plugins because Closure interfaces cannot accept Kotlin's lambdas.However, we need use Java to provide such interfaces because Gradle 6.7 or lower do properly recognize such interface that are originally written in Groovy. ref: https://github.com/jmatsu/repro-kotlin-dsl-bug-gradle-plugin-written-in-groovy/blob/master/README.md
This PR includes the following changes.