Add support for a "Show Documentation" quick fix menu entry #789
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This requires a corresponding PR to VSCode to handle the new command: PowerShell.ShowCodeActionDocumentation Changed the use of the Diagnostic.Code field to hold the PSSA rule name. According to the LSP docs, this field is meant to be displayed and tslint uses it to display the vioated rule. Update the logic to use a combination of diagnostic fields to get a unique id in order to retrieve the stored "correction edits" when the textDocument/codeAction message is processed with the diagnostic array returned earlier. Also, observed a few times during startup that RunScriptDiagnostics was getting called with an empty filesToAnalyze array. Modified to return early in that case. Remove a private overload of DelayThenInvokeDiagnostics that wasn't being used.
rkeithhill
added a commit
to PowerShell/vscode-powershell
that referenced
this pull request
Nov 4, 2018
This corresponds to PSES PR: PowerShell/PowerShellEditorServices#789 Currently this points to the PSSA development branch. Maybe that should point to the master branch. Could be a setting I suppose but that seems like overkill.
4 tasks
rjmholt
approved these changes
Nov 6, 2018
There was a problem hiding this comment.
LGTM! Thanks @rkeithhill. Left a minor comment, but I'm good to go as is :)
| markerIndex.TryGetValue(diagnostic.Code, out correction)) | ||
| if (string.IsNullOrEmpty(diagnostic.Code)) | ||
| { | ||
| this.Logger.Write( |
| private static Diagnostic GetDiagnosticFromMarker(ScriptFileMarker scriptFileMarker) | ||
| { | ||
| return new Diagnostic | ||
| { | ||
| Severity = MapDiagnosticSeverity(scriptFileMarker.Level), | ||
| Message = scriptFileMarker.Message, | ||
| Code = scriptFileMarker.Source + Guid.NewGuid().ToString(), | ||
| Code = scriptFileMarker.RuleName, |
| @@ -1182,6 +1183,7 @@ private bool IsQueryMatch(string query, string symbolName) | |||
| return symbolName.IndexOf(query, StringComparison.OrdinalIgnoreCase) >= 0; | |||
| } | |||
|
|
|||
| // https://microsoft.github.io/language-server-protocol/specification#textDocument_codeAction | |||
There was a problem hiding this comment.
omg we should have comments like this all over... so great! (not in this PR 😄 )
Also update codeAction processing to ensure we don't have multiple show documentation commands for the same rule.
|
Addressed all the current PR change requests. |
|
Pinging @TylerLeonhardt for approval |
|
@TylerLeonhardt, nice username btw 😉 |
SeeminglyScience
approved these changes
Nov 13, 2018
| sb.Append("-"); | ||
| sb.Append(end.Line); | ||
| sb.Append(":"); | ||
| sb.Append(end.Character); |
There was a problem hiding this comment.
Consider chaining these, e.g.
return new StringBuilder(256)
.Append(diagnostic.Source ?? "?")
.Append("_")
.Append(diagnostic.Code ?? "?")
.Append("_")
.Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?")
.Append("_")
.Append(start.Line)
.Append(":")
.Append(start.Character)
.Append("-")
.Append(end.Line)
.Append(":")
.Append(end.Character)
.ToString();There was a problem hiding this comment.
small nit... it might be useful to do:
StringBuilder sb = new StringBuilder(256)
.Append(diagnostic.Source ?? "?")
.Append("_")
.Append(diagnostic.Code ?? "?")
.Append("_")
.Append((diagnostic.Severity != null) ? diagnostic.Severity.ToString() : "?")
.Append("_")
.Append(start.Line)
.Append(":")
.Append(start.Character)
.Append("-")
.Append(end.Line)
.Append(":")
.Append(end.Character)
return sb.ToString();so that we can throw a breakpoint on the return line and see what the contents of sb are. But I won't block on doing that vs what Patrick suggested
There was a problem hiding this comment.
Heh, I was just going to suggest this very thing. :-) Will fix.
There was a problem hiding this comment.
Honestly my biggest desire is for a function-return-variable in debug view.
rjmholt
reviewed
Nov 13, 2018
TylerLeonhardt
approved these changes
Nov 13, 2018
rkeithhill
added a commit
to PowerShell/vscode-powershell
that referenced
this pull request
Nov 15, 2018
This corresponds to PSES PR: PowerShell/PowerShellEditorServices#789 Currently this points to the PSSA development branch. Maybe that should point to the master branch. Could be a setting I suppose but that seems like overkill.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires a corresponding PR to VSCode to handle the new command:
PowerShell.ShowCodeActionDocumentation
Changed the use of the Diagnostic.Code field to hold the PSSA rule name.
According to the LSP docs, this field is meant to be displayed and
tslint uses it to display the vioated rule. Update the logic to use a
combination of diagnostic fields to get a unique id in order to
retrieve the stored "correction edits" when the
textDocument/codeAction message is processed with the diagnostic
array returned earlier.
Also, observed a few times during startup that RunScriptDiagnostics was
getting called with an empty filesToAnalyze array. Modified to return
early in that case.
Remove a private overload of DelayThenInvokeDiagnostics that wasn't
being used.