-
Notifications
You must be signed in to change notification settings - Fork 234
Change Get-Help behavior to return local help when no online help available #721
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
Changes from 12 commits
c35d13e
75f8afb
55936d1
6ad4e91
51a2ae8
0f65dc6
74f04ff
c772aa0
4090e8e
c481d02
d125d66
f100131
c2ba5ad
504a5a8
004d53d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,18 @@ | |
|
||
namespace Microsoft.PowerShell.EditorServices.Protocol.LanguageServer | ||
{ | ||
// We don't expect ShowOnlineHelpRequest to come from vscode anymore, but it could come from another editor. | ||
// TODO: Note that it's deprecated if it's called??? | ||
public class ShowOnlineHelpRequest | ||
{ | ||
public static readonly | ||
RequestType<string, object, object, object> Type = | ||
RequestType<string, object, object, object>.Create("powerShell/showOnlineHelp"); | ||
} | ||
public class ShowHelpRequest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: the indentation of this line is a bit off |
||
{ | ||
public static readonly | ||
RequestType<string, object, object, object> Type = | ||
RequestType<string, object, object, object>.Create("powerShell/showHelp"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,6 +122,7 @@ public void Start() | |
this.HandleDocumentRangeFormattingRequest); | ||
|
||
this.messageHandlers.SetRequestHandler(ShowOnlineHelpRequest.Type, this.HandleShowOnlineHelpRequest); | ||
this.messageHandlers.SetRequestHandler(ShowHelpRequest.Type, this.HandleShowHelpRequest); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add newlines around this line to keep the style consistent? |
||
this.messageHandlers.SetRequestHandler(ExpandAliasRequest.Type, this.HandleExpandAliasRequest); | ||
|
||
this.messageHandlers.SetRequestHandler(FindModuleRequest.Type, this.HandleFindModuleRequest); | ||
|
@@ -230,22 +231,45 @@ await requestContext.SendResult( | |
} | ||
}); | ||
} | ||
|
||
protected async Task HandleShowOnlineHelpRequest( | ||
protected async Task HandleShowHelpRequest( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: newline between functions |
||
string helpParams, | ||
RequestContext<object> requestContext) | ||
{ | ||
if (helpParams == null) { helpParams = "get-help"; } | ||
|
||
var psCommand = new PSCommand(); | ||
psCommand.AddCommand("Get-Help"); | ||
psCommand.AddArgument(helpParams); | ||
psCommand.AddParameter("Online"); | ||
|
||
await editorSession.PowerShellContext.ExecuteCommand<object>(psCommand); | ||
|
||
const string CheckHelpScript = @" | ||
[CmdletBinding()] | ||
param ( | ||
[String]$CommandName | ||
) | ||
try { | ||
$null = Microsoft.PowerShell.Core\Get-Command $CommandName -ErrorAction Stop | ||
} catch [System.Management.Automation.CommandNotFoundException] { | ||
$PSCmdlet.ThrowTerminatingError($PSItem) | ||
} | ||
try { | ||
$null = Microsoft.PowerShell.Core\Get-Help $CommandName -Online | ||
} catch [System.Management.Automation.PSInvalidOperationException] { | ||
Microsoft.PowerShell.Core\Get-Help $CommandName -Full | ||
}"; | ||
if (string.IsNullOrEmpty(helpParams)) { helpParams = "Get-Help"; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a newline above this |
||
|
||
PSCommand checkHelpPSCommand = new PSCommand() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was about to complain about the lack of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used PSCommand due to all the other comments about being explicit about the variable type 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wholly agree. In general you wouldn't lose explicit variable type constraints with |
||
.AddScript(CheckHelpScript, useLocalScope: true) | ||
.AddArgument(helpParams); | ||
await editorSession.PowerShellContext.ExecuteCommand<PSObject>(checkHelpPSCommand, sendOutputToHost: true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a newline above the first |
||
await requestContext.SendResult(null); | ||
} | ||
protected async Task HandleShowOnlineHelpRequest( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely add a newline before this method definition here |
||
string helpParams, | ||
RequestContext<object> requestContext | ||
) | ||
{ | ||
const string deprecatedWarning = @" | ||
Write-Warning ""'powerShell/showOnlineHelp' has been deprecated. Use 'powerShell/showHelp' instead."""; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooh, does this show in the Integrated Console? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does 😃 it's due to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of pwsh.AddCommand("Microsoft.PowerShell.Utility\\Write-Verbose")
.AddParameter("Message", "'powerShell/showOnlineHelp' has been deprecated. Use 'powerShell/showHelp instead."); Using command instead allows it to create a command processor for that command specifically, instead of having to parse a script. |
||
PSCommand commandDeprecated = new PSCommand() | ||
.AddScript(deprecatedWarning); | ||
await editorSession.PowerShellContext.ExecuteCommand<PSObject>(commandDeprecated, sendOutputToHost: true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of (or in addition to) a PSIC warning, we could log a warning to the log file. I could go either way on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The ideal would be to launch a popup. But that would be a lot for this PR. Perhaps instead we can leave it as a PSIC message and open an issue to make a popup? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a newline above this |
||
await this.HandleShowHelpRequest(helpParams, requestContext); | ||
} | ||
|
||
private async Task HandleSetPSSARulesRequest( | ||
object param, | ||
|
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 might be able to mark this with the
[Obsolete]
attribute :)