-
Notifications
You must be signed in to change notification settings - Fork 273
telemetry: add ide diagnostics for inline #5862
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
Conversation
759b773
to
7f25c1c
Compare
if (isInternalUser(getStartUrl(project))) { | ||
val oldDiagnostics = sessionContext.diagnostics.orEmpty() | ||
// wait for the IDE itself to update its diagnostics for current file | ||
delay(500) |
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 will reflect in metrics
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 if diagnostics run takes longer than 500ms?
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.
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.
There is no callback provided by JetBrains for "diagnostic updated complete" once the acceptance is done. Meanwhile, user can also do quick keyboard input immediately after acceptance.
We ack two issues with the implementation:
- If IDE itself does not provide diagnostics (like VSC), we will return [] in diagnostics.
- If IDE cannot finish within 500ms, we will return [] in diagnostics.
There is no workaround for #1 because we cannot force user to install the LSP for that language.
For #2, 500ms is a best middle ground between "too fast to see any diagnostic showing up" and "too slow to be interrupted by next user input".
severity == HighlightSeverity.WARNING || | ||
severity == HighlightSeverity.WEAK_WARNING -> "WARNING" | ||
severity == HighlightSeverity.INFORMATION -> "INFORMATION" | ||
severity.toString().contains("TEXT", ignoreCase = true) -> "HINT" |
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.
severity.toString().contains("TEXT", ignoreCase = true) -> "HINT" | |
severity.name == "TEXT" -> "HINT" |
?
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 think this changed the original logic right? It was trying to look contains not exact match
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.
my question is if we can do equality or does it need to be a contains check
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.
?.key ?: "OTHER" | ||
} | ||
|
||
fun convertSeverity(severity: HighlightSeverity): String = when { |
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.
fun convertSeverity(severity: HighlightSeverity): String = when { | |
fun convertSeverity(severity: HighlightSeverity): DiagnosticSeverity = when { |
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 fix
val diagnosticPatterns = mapOf( | ||
"TYPE_ERROR" to listOf("type", "cast"), | ||
"SYNTAX_ERROR" to listOf("expected", "indent", "syntax"), | ||
"REFERENCE_ERROR" to listOf("undefined", "not defined", "undeclared", "reference", "symbol"), | ||
"BEST_PRACTICE" to listOf("deprecated", "unused", "uninitialized", "not initialized"), | ||
"SECURITY" to listOf("security", "vulnerability") | ||
) |
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.
extract so we are not constantly rebuilding nested data structures
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 fix
61786c2
to
68c86f1
Compare
31ea891
to
bd6f8c7
Compare
Types of changes
Description
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.