Skip to content

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

Merged
merged 15 commits into from
Sep 21, 2018
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -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???
Copy link
Contributor

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 :)

public class ShowOnlineHelpRequest
{
public static readonly
RequestType<string, object, object, object> Type =
RequestType<string, object, object, object>.Create("powerShell/showOnlineHelp");
}
public class ShowHelpRequest
Copy link
Member

Choose a reason for hiding this comment

The 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");
}
}
46 changes: 35 additions & 11 deletions src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public void Start()
this.HandleDocumentRangeFormattingRequest);

this.messageHandlers.SetRequestHandler(ShowOnlineHelpRequest.Type, this.HandleShowOnlineHelpRequest);
this.messageHandlers.SetRequestHandler(ShowHelpRequest.Type, this.HandleShowHelpRequest);
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -230,22 +231,45 @@ await requestContext.SendResult(
}
});
}

protected async Task HandleShowOnlineHelpRequest(
protected async Task HandleShowHelpRequest(
Copy link
Member

Choose a reason for hiding this comment

The 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"; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a newline above this if


PSCommand checkHelpPSCommand = new PSCommand()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to complain about the lack of var here, but technically the type here is the return type of the method. So all I feel is ambivalence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 😁

Copy link
Contributor

@rjmholt rjmholt Sep 6, 2018

Choose a reason for hiding this comment

The 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 var when the constructor is on the RHS of the assignment (var x = new Banana();), but here it's more nuanced because of a sort of Builder pattern scenario...

.AddScript(CheckHelpScript, useLocalScope: true)
.AddArgument(helpParams);
await editorSession.PowerShellContext.ExecuteCommand<PSObject>(checkHelpPSCommand, sendOutputToHost: true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a newline above the first await statement

await requestContext.SendResult(null);
}
protected async Task HandleShowOnlineHelpRequest(
Copy link
Contributor

Choose a reason for hiding this comment

The 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.""";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, does this show in the Integrated Console?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does 😃 it's due to sendOutputToHost: true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of AddScript you could use AddCommand here instead.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@rjmholt rjmholt Sep 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on log file Actually, users won't read the log file, and it won't be relevant when looking back through the logs. We should warn the user somehow.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a newline above this await

await this.HandleShowHelpRequest(helpParams, requestContext);
}

private async Task HandleSetPSSARulesRequest(
object param,
Expand Down