[INFRA] Improve the java style checks log the errors to sbt console #3115
Conversation
build.sbt
Outdated
| if (errors.nonEmpty) { | ||
| errors.foreach { case (file, line, message) => | ||
| log.error(s"- File: $file, Line: $line, Message: $message") | ||
| } | ||
| sys.error(s"Found checkstyle errors") | ||
| } |
There was a problem hiding this comment.
It looks good to me. The only request is, can we print the errors as part of the failure message? That way we don't need to scroll up to find the error.
var errorMsg = "Found checkstyle errors"
errors.foreach { case (file, line, message) =>
val lineError = s"File: $file, Line: $line, Message: $message"
log.error(lineError)
errorMsg += ("\n" + lineError)
}
sys.error(errorMsg + "\n")
There was a problem hiding this comment.
@vkorukanti Thanks for reviewing.
build.sbt
Outdated
| checkstyleSeverityLevel := CheckstyleSeverityLevel.Error, | ||
| (Compile / compile) := ((Compile / compile) dependsOn (Compile / checkstyle)).value, | ||
| (Test / test) := ((Test / test) dependsOn (Test / checkstyle)).value | ||
| checkstyleSeverityLevel := CheckstyleSeverityLevel.Ignore, |
There was a problem hiding this comment.
one naive qn: why is this changed from Error to Ignore? Is it because we want to control the part about throwing error?
There was a problem hiding this comment.
Is it because we want to control the part about throwing error?
@vkorukanti Yes, that's right. If we keep the Error severity, build/sbt will throw an error and immediately stop at the checkstyle phase (if error) -> never execute the check-report phase of checkstyle-report.xml and checkstyle-test-report.xml. We need to Ignore and throw error if exists when checking *report.xml.
build.sbt
Outdated
| @@ -1335,18 +1335,51 @@ lazy val scalaStyleSettings = Seq( | |||
| **************************** | |||
| */ | |||
|
|
|||
| lazy val compileJavastyle = taskKey[Unit]("compileJavastyle") | |||
| lazy val testJavastyle = taskKey[Unit]("testJavastyle") | |||
|
|
|||
| def javaCheckstyleSettings(checkstyleFile: String): Def.SettingsDefinition = { | |||
There was a problem hiding this comment.
Thanks for doing this super helpful! Wondering if we should move all this code to a separate file since it's now more than just a few lines?
There was a problem hiding this comment.
@allisonport-db Thanks for reviewing. Your point sounds good, we have a quite complicated build.sbt now. But if isolating this checkstyle part to a separate file, where should we organize that file?
There was a problem hiding this comment.
We keep some of the build-related code (e.g., Unidos) under the project directory. You can put it there.
…bt console Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
@vkorukanti It's nice. TYSM! |
…elta-io#3115) ## Description Resolves delta-io#3067. ## How was this patch tested? On local machine, intentionally create checkstyle errors in module `kernelDefaults` (for experimental), then run the `build/sbt compile` and `build/sbt kernelDefaults/test`. Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
…elta-io#3115) Resolves delta-io#3067. On local machine, intentionally create checkstyle errors in module `kernelDefaults` (for experimental), then run the `build/sbt compile` and `build/sbt kernelDefaults/test`. Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
Which Delta project/connector is this regarding?
Description
Resolves #3067.
How was this patch tested?
On local machine, intentionally create checkstyle errors in module
kernelDefaults(for experimental), then run thebuild/sbt compileandbuild/sbt kernelDefaults/test.Does this PR introduce any user-facing changes?
No.