-
Notifications
You must be signed in to change notification settings - Fork 121
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
Ignored javadoc tool error #448
Conversation
e7f0cee
to
142f7aa
Compare
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.
Running the cross-module validator to check if we can get rid of JavacLogger#flush(Int)
.
synchronized { | ||
val parser = new JavaErrorParser(cwd) | ||
|
||
parser.parseProblems(err, log).foreach(reporter.log(_)) |
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.
Is this right? The old impl interleaved \n
's between error messages.
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.
Hmm, you're right - probably makes sense to add those newlines in err(s: => String)
(and add some more tests describing how this is actually called).
val parser = new JavaErrorParser(cwd) | ||
|
||
parser.parseProblems(err, log).foreach(reporter.log(_)) | ||
out.split("\n").filter(_ != "").foreach(log.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.
Are you purposely splitting info messages by newlines to send them independently to log.info?
Why not, instead of concat-ing info messages and then re-splitting, just collect the messages into a list buffer and then iterate that list into log.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.
Hmm, for some reason I got the impression that we couldn't assume out
would be called in neatly framed chunks, but indeed looking at the previous implementation again it appears that we in fact can. In that case a ListBuffer
indeed makes more sense, I'll update accordingly!
The validator has checked the following projects against Scala 2.12,
✅ The result is: SUCCESS |
parser.parseProblems(input, log).foreach(reporter.log) | ||
} | ||
|
||
def flush(exitCode: Int): Unit = { |
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 like we can drop this method, but we need to tell MiMa to ignore it.
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.
LGTM, thanks @raboof.
6283df4
to
6b4af5a
Compare
(rebased, unfortunately it seems that triggered github to 'dismiss' the review :/) |
Hey @raboof thanks! Can you squash all the commits? The previous commits to the formatting are not passing in the CI, and it seems that this fix could be all in one commit. |
When the javadoc tool fails with an error that could not be parsed, the error was silently dropped.
6b4af5a
to
c8c20ac
Compare
Sure, done! |
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 @raboof 😄
Reported under #415