-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Reimplement reporting error and warning count when a compilation error happens #29183
base: master
Are you sure you want to change the base?
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.
Looks good. I have only a few questions for my own education and some naming/documentation-related requests.
.../src/integTest/groovy/org/gradle/api/tasks/compile/JavaCompileProblemsIntegrationTest.groovy
Show resolved
Hide resolved
...ge-java/src/main/java/org/gradle/api/internal/tasks/compile/DiagnosticToProblemListener.java
Outdated
Show resolved
Hide resolved
/** | ||
* This method is based on {JavaCompiler#printCount()} | ||
*/ |
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 javadoc should state the following:
- functionality: this method prints the number of warnings / errors to the standard output
- intent: when there's no diagnostic listener attached to the compiler object, the number of warnings/errors are part of the putput. Since we have a listener attached, the output changed. Calling this method makes restores compatiblity with the original output
- source: the method implementation is based on {JavaCompiler#printCount()}.
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.
Added further documentation
@@ -63,6 +63,7 @@ public WorkResult execute(JavaCompileSpec spec) { | |||
ApiCompilerResult result = new ApiCompilerResult(); | |||
JavaCompiler.CompilationTask task = createCompileTask(spec, result); | |||
boolean success = task.call(); | |||
diagnosticToProblemListener.reportDiagnosticCounts(); |
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.
❓ Will we hit this code path when we do joint Java-Groovy compilation?
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.
Added a new integration test verifying this
...y/src/integTest/groovy/org/gradle/groovy/compile/GroovyCompileProblemsIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
...y/src/integTest/groovy/org/gradle/groovy/compile/GroovyCompileProblemsIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
* /.../Bar.java:10: warning: [cast] redundant cast to String | ||
* String s = (String)"Hello World"; | ||
* ^ | ||
* 1 error |
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.
Just to validate: do we have coverage when we have 0 errors and/or 1 warnings on the output?
387f400
to
193c3f7
Compare
@bot-gradle test this |
I've triggered the following builds for you. Click here to see all build failures. |
When the new problem reporting framework was introduced, we didn't yet implement the error counters.
E.g., the following output from
javac
has a count at the end (note on the bottom the counters)This PR adds these counters back.