-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add error reports info to metals doctor #5683
Conversation
4ba2fda
to
51553de
Compare
51553de
to
5f05763
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.
This is a nice idea! I left a couple small comments. Since we're also changing the structure of the doctor output here we'll also want to bump the DoctorResults.version
.
metals/src/main/scala/scala/meta/internal/metals/doctor/DoctorResults.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/doctor/DoctorResults.scala
Show resolved
Hide resolved
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.
Great work! I'm wondering if for reports which contain scala code in them (eg. CompilerAccess.handleError
), can we wrap code in
```scala
...
```
?
.map(decode) | ||
.mkString("\n") | ||
} | ||
rc.getReports().map { case TimestampedFile(file, timestamp) => |
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.
Would it be possible to instead use the target name in the filename? Similar to how we do it with timestamp?
@@ -20,7 +20,7 @@ final case class DoctorResults( | |||
|
|||
object DoctorResults { | |||
// Version of the Doctor json that is returned. | |||
val version = 4 | |||
val version = 5 |
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.
Should we bump metals to 1.1.x in this case? What do you think @ckipp01 ?
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 https://scalameta.org/metals/docs/contributors/releasing#tag-the-release probably yea.
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.
We will also try to explain all the recent changes in a plugin authors paragraph then
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, just a few comments
mtags/src/main/scala-2/scala/meta/internal/pc/ScalaPresentationCompiler.scala
Show resolved
Hide resolved
name <- buildTargets | ||
.scalaTarget(buildTargetId) | ||
.map(_.displayName) | ||
.orElse(buildTargets.javaTarget(buildTargetId).map(_.displayName)) |
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.
name <- buildTargets | |
.scalaTarget(buildTargetId) | |
.map(_.displayName) | |
.orElse(buildTargets.javaTarget(buildTargetId).map(_.displayName)) | |
name <- buildTargets | |
.info(buildTargetId) | |
.map(_.displayName)) |
should be enough
} | ||
|
||
case class DoctorVisibilityDidChangeParams( | ||
visible: Boolean | ||
) | ||
|
||
object Doctor { | ||
def getErrorReportSummary( |
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.
Could you explain a bit what this does in a comment?
*/ | ||
final case class ErrorReportInfo( | ||
name: String, | ||
timestamp: Long, |
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'm sending a timestamp but I wonder if it'd be better to send nicely formatted time-date string instead.
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! Just one comment.
reversed = lines.reverse | ||
index = reversed.indexWhere(_.startsWith(Report.summaryTitle)) | ||
if index >= 0 | ||
} yield reversed | ||
.slice(0, index) | ||
.reverse | ||
.dropWhile(_ == "") | ||
.map(decode) | ||
.mkString("\n") |
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.
reversed = lines.reverse | |
index = reversed.indexWhere(_.startsWith(Report.summaryTitle)) | |
if index >= 0 | |
} yield reversed | |
.slice(0, index) | |
.reverse | |
.dropWhile(_ == "") | |
.map(decode) | |
.mkString("\n") | |
index = lines.indexWhere(_.startsWith(Report.summaryTitle)) | |
if index >= 0 | |
} yield lines | |
.slice(index, lines.length) | |
.takeWhile(_ != "") | |
.map(decode) | |
.mkString("\n") |
do we need reverse at all?
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.
You can also use lastIndexWhere
if needed.
resolves: #5672