-
Notifications
You must be signed in to change notification settings - Fork 28
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
Gradle Config DSL #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marcoferrer)
community-scripts/extendable-messages/build.gradle, line 4 at r2 (raw file):
plugins { id "org.jetbrains.kotlin.jvm" version "1.3.0"
update to 1.3.60?
community-scripts/grpc-gateway-entry-point/build.gradle, line 14 at r2 (raw file):
dependencies{ implementation "com.github.marcoferrer.krotoplus:protoc-gen-kroto-plus:$krotoPlusVersion" implementation "com.google.protobuf:protobuf-java:3.6.1"
update to 3.10.x ?
example-project/build.gradle, line 24 at r2 (raw file):
dependencies{ classpath "com.github.marcoferrer.krotoplus:kroto-plus-gradle-plugin:${versions.krotoplus}" classpath "com.google.protobuf:protobuf-java:${versions.protobuf}"
I like how clean this methodology is (${versions.protobuf
}), but kotlin-dsl seems to block this way of referencing the ext system, especially in the plugins block. Is that part of why Kroto Plus uses the groovy DSL?
example-project/build.gradle, line 38 at r2 (raw file):
apply plugin: "com.github.marcoferrer.kroto-plus" krotoPlus {
I love how much nicer it is to have it in here like other plugins (such as protobuf config).
example-project/build.gradle, line 52 at r2 (raw file):
} //add overloads for file path
time for a Trello?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marcoferrer and @mattdkerr)
example-project/build.gradle, line 24 at r2 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
I like how clean this methodology is (
${versions.protobuf
}), but kotlin-dsl seems to block this way of referencing the ext system, especially in the plugins block. Is that part of why Kroto Plus uses the groovy DSL?
For ktdsl scripts you can get the same behavior by defining versions as consts in buildSrc. The kroto build currently does this. The project still uses the groovy dsl because when it was started the ktdsl wasn't as mature as I would have liked. There was a ton of bugs in the IDE integration as well. A few plugins were pretty painful to use, protobuf gradle being one of them. I submitted the kotlin dsl support to protobuf gradle but was still waiting for ktdsl to stabilize a little more in general. Fast forward to today and I think its matured to a point where its probably time to migrate
example-project/build.gradle, line 38 at r2 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
I love how much nicer it is to have it in here like other plugins (such as protobuf config).
Im really looking forward to publishing this. I feel like it simplifies some of the pain points users have with configuration management
example-project/build.gradle, line 52 at r2 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
time for a Trello?
Havent used it before but I can give it a shot
89ef739
to
acf0ccf
Compare
acf0ccf
to
198b8b9
Compare
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
============================================
+ Coverage 85.29% 86.53% +1.24%
Complexity 19 19
============================================
Files 15 15
Lines 306 312 +6
Branches 49 53 +4
============================================
+ Hits 261 270 +9
+ Misses 16 15 -1
+ Partials 29 27 -2
Continue to review full report at Codecov.
|
@@ -0,0 +1,114 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file introduces preliminary work needed for simplifying protobuf configuration for generic use cases
I still need to fix the repository configuration in the example project. @mattdkerr Let me know what your impressions are about the kroto config dsl and it’s usage. The example project build file shows how to create a config and apply it to protobuf Gradle. This was the last thing I wanted to get into the 0.6.0 release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 18 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @marcoferrer)
.travis.yml, line 24 at r4 (raw file):
script: - ./gradlew check - cd example-project && ./gradlew test && ./gradlew clean test -PuseKrotoConfigDsl=true
is this to be explicit? it would be nice to not have to pass anything in (make it the default)
example-project/build.gradle, line 99 at r4 (raw file):
mockServices { filter { addIncludePath("jojo/*")
would you add multiple by array or by multiple lines of addIncludePath ?
example-project/build.gradle, line 107 at r4 (raw file):
} mockServices {
it's good to see an example of how to define more than one- this is useful
example-project/build.gradle, line 116 at r4 (raw file):
} protoBuilders {
does the same hold true for protoBuilders as for mockServices? would you want to make a new protoBuilders section for another imported dependency that holds proto files, for example? I was running into issues doing this with a zipped proto artifact on a private SonaType server, where it wouldn't property generate builders for everything and would then fail out (if I removed the explicit filter path for addInclude on the other area it would not make the DSL builders from Kroto and worked fine again)
example-project/build.gradle, line 147 at r4 (raw file):
entry { point = 'MESSAGE_IMPLEMENTS' addScriptPath "extendableMessages.kts"
I don't have a handle on these yet, so I can't comment- but it looks like some these has Groovy syntax. I am using Kotlin DSL in Gradle, so the addScriptPath(...) approach looks more familiar to me.
example-project/build.gradle, line 153 at r4 (raw file):
point = 'BUILDER_IMPLEMENTS' addScriptPath "extendableMessages.kts" scriptBundle = file("kp-scripts/build/libs/kp-scripts.jar")
in Kotlin DSL this would be File(...) I believe
kroto-plus-gradle-plugin/build.gradle, line 31 at r4 (raw file):
compileOnly gradleApi() compileOnly "org.gradle:gradle-kotlin-dsl:1.0.4" compileOnly "com.google.protobuf:protobuf-gradle-plugin:0.8.8"
doesn't match above where 0.8.7 is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @marcoferrer and @mattdkerr)
.travis.yml, line 24 at r4 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
is this to be explicit? it would be nice to not have to pass anything in (make it the default)
This is purely for testing compilation against both configuration types. It wont be needed for actual dsl usage.
example-project/build.gradle, line 99 at r4 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
would you add multiple by array or by multiple lines of addIncludePath ?
Since this backed by a protobuf message builder it exposes the same java methods you would expect for building collections. You can call addIncludePath
multiple times or addAllIncludePath
with a collection.
example-project/build.gradle, line 116 at r4 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
does the same hold true for protoBuilders as for mockServices? would you want to make a new protoBuilders section for another imported dependency that holds proto files, for example? I was running into issues doing this with a zipped proto artifact on a private SonaType server, where it wouldn't property generate builders for everything and would then fail out (if I removed the explicit filter path for addInclude on the other area it would not make the DSL builders from Kroto and worked fine again)
Yes each generator allows multiple configurations to be supplied. Its meant to allow users to use different generator options for different sources.
example-project/build.gradle, line 153 at r4 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
in Kotlin DSL this would be File(...) I believe
I think there is a difference between the two. In this case file
has an implicit receiver of the build scripts project. So technically the fully qualified method is project.file(...)
which uses the project directory as the parent of the newly created file instance. File
in kotlin dsl would be the same as new File(...)
kroto-plus-gradle-plugin/build.gradle, line 31 at r4 (raw file):
Previously, mattdkerr (Matt Kerr) wrote…
doesn't match above where 0.8.7 is used
Good catch
This PR is for issue #8
This change is