-
Notifications
You must be signed in to change notification settings - Fork 0
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
Separate modules for plugin and ProtoData model #3
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.
@Oleg-Melnik please see my comments.
Also, "ProtoData" is a name of the library. I have fixed the original PR title, but please address the typo in the PR description.
.idea/live-templates/README.md
Outdated
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.
Let's put this file back.
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.
Done.
model/build.gradle.kts
Outdated
/** | ||
* The alias for typed extensions functions related to modules of this project. | ||
*/ | ||
typealias Module = Project |
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.
Most (if not all) of the settings from here down, are applicable to both Gradle modules. Let's move them back to the root build.gradle.kts
, and apply them to sub-projects.
Ideally, model
should just speak of Protobuf settings and ProtoData.
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.
Done.
@Oleg-Melnik Let's fix the PR description: we don't have any "prototype definitions". We have Protobuf definitions. |
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.
@Oleg-Melnik LGTM with some minor comments to address prior to merging.
build.gradle.kts
Outdated
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile | ||
|
||
object BuildSettings { |
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.
In Gradle, buildscript
section is a special one. It tells how to perform the build itself in terms of classpath etc. One should think of it as of a configuration for Gradle, so to speak. It is also isolated from the rest of the build in classpath, imports and variables.
In plugins
sections our allowed actions are also limited. For instance, AFAIK, we cannot use external variables inside of it.
Therefore I suggest to move BuildSettings
to go even after plugins
, right before the actual code which we write to describe the build stages.
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.
Done.
build.gradle.kts
Outdated
} | ||
|
||
/** | ||
* The alias for typed extensions functions related to modules of this project. | ||
*/ | ||
typealias Module = Project | ||
|
||
project.run { | ||
fun Module.applyConfiguration() { | ||
configureJava() | ||
configureKotlin() | ||
setupTests() |
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.
I think, that's a typo in the original PR. It should be setUpTests
(where "set up" is a verb), not setup
(where "setup" is a noun).
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.
Done.
This PR introduces two separate modules: one for the Protobuf definitions and the other for the plugin itself. This is necessary because the ProtoData plugin should be compiled and deployed before the ProtoData model can be processed.