-
Notifications
You must be signed in to change notification settings - Fork 13
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
Convert build scripts to Kotlin #396
base: master
Are you sure you want to change the base?
Conversation
def sources = (sourceSets.main.java).getFiles().collect({ src -> src.getPath() }).sort() | ||
def sourcesStr = sources.inject(null, { acc, source -> acc ? acc + " " + source : source }) | ||
|
||
def proc = "etags ${sourcesStr} ".execute() |
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.
java.io.IOException: Cannot run program "etags": error=2, No such file or directory
seems to be an existing issue.
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.
Does this only happen when this target is run or always? Previously, this target had to be run manually.
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.
Thanks for investigating this! I have a bunch of questions and comments.
Did you manually convert this or is there some IDE/tool support?
The Kotlin version looks a bit more verbose, but if this provides better IDE support, I'm fine with that. Overall, I don't find it much more readable. Do you?
build.gradle
Outdated
implementation files("${checkerJar}") | ||
implementation group: 'com.google.errorprone', name: 'javac', version: "$errorproneJavacVersion" | ||
implementation(files(checkerJar)) | ||
implementation(group = "com.google.errorprone", name = "javac", version = errorproneJavacVersion) |
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.
What's the difference between this line and line 52? Not related to the conversion, it just looks odd.
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.
The javacJar
is a configuration that is not only a dependency but also used in other places. I have updated its declaration to improve the syntax and make it more reusable.
build.gradle
Outdated
val mils = result.endTime - result.startTime | ||
val seconds = mils / 1000.0 | ||
|
||
logger.info( |
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.
Before this used println
, now it uses logger.info
... Is there a difference? Should other prinlns be replaced? Are the loggers configured somewhere?
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.
According to [Writing your own log messages], it seems println
is equivalent to logger.quiet
. I have changed it back to println
for consistency.
def sources = (sourceSets.main.java).getFiles().collect({ src -> src.getPath() }).sort() | ||
def sourcesStr = sources.inject(null, { acc, source -> acc ? acc + " " + source : source }) | ||
|
||
def proc = "etags ${sourcesStr} ".execute() |
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.
Does this only happen when this target is run or always? Previously, this target had to be run manually.
@wmdietl For |
Instead of using Groovy, a dynamically typed language, we should use Kotlin, a statically typed language.
Pros:
Cons: