Skip to content
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

fix #1393: Always use local help to return Full help #1394

Conversation

deadlydog
Copy link
Contributor

@deadlydog deadlydog commented Nov 24, 2020

If the user has overridden the Get-Help -Online parameter to always be true, then the ShowHelpHandler will always launch a new browser window to the cmdlet help.
Instead, we want it to return the full cmdlet text as intended.

fixes #1393
fixes PowerShell/vscode-powershell#3071

@deadlydog deadlydog requested a review from rjmholt as a code owner November 24, 2020 06:26
@TylerLeonhardt
Copy link
Member

I'm curious... does that actually fix your issue?

I would have guessed that you needed to add it here:

@TylerLeonhardt
Copy link
Member

What you've changed is what gets run by the Command Explorer added by @corbob which only gets fired if you hit this little question mark:

image

@TylerLeonhardt
Copy link
Member

Also just want to say, thank you so much for contributing. I love it when folks open issues and then submit PRs! You're always welcome here!

@deadlydog
Copy link
Contributor Author

Thanks for the quick response on this @TylerLeonhardt :)

You may very well be right that I applied the fix in the wrong spot. I read the Development docs on the ReadMe and the Contribution Guidelines, but neither of them told me how to test the changes. I was able to build the project file as the guide suggested, but wasn't sure how to test the changes. Perhaps there are docs on how to do that and I just overlooked them?

I was also hoping to add some unit tests around the new functionality, but could not get the test projects to build. This is what I tried:

  • In VS 2019: The build fails trying to build the PowerShellEditorServices.TestHost project for net461 and netcoreapp2.1 saying

    CS0234 The type or namespace name 'Protocol' does not exist in the namespace 'Microsoft.PowerShell.EditorServices' (are you missing an assembly reference?) PowerShellEditorServices.Test.Host (net461), PowerShellEditorServices.Test.Host (netcoreapp2.1) C:\dev\Git\3rdParty\PowerShellEditorServices\test\PowerShellEditorServices.Test.Host\DebugAdapterTests.cs 6 Active

  • In my local VS Code: The Run Build Task succeeds, but the Run Test Task fails with a bunch of similar errors to:

    C:\dev\Git\3rdParty\PowerShellEditorServices\test\PowerShellEditorServices.Test.Host\DebugAdapterTests.cs(6,43): error CS0234: The type or namespace name 'Protocol' does not exist in the namespace 'Microsoft.PowerShell.EditorServices' (are you missing an assembly reference?) [C:\dev\Git\3rdParty\PowerShellEditorServices\test\PowerShellEditorServices.Test.Host\PowerShellEditorServices.Test.Host.csproj]

  • And in the VS Code dev container: The Run Build Task fails with:

    ### Installation complete for version . .NET installation complete ERROR: Cannot find path '/workspaces/PowerShellEditorServices/.dotnet/dotnet' because it does not exist. At /workspaces/PowerShellEditorServices/PowerShellEditorServices.build.ps1:116 char:25 + $script:dotnetExe = Resolve-Path $dotnetExePath

I do have dotnet.exe on my Windows PATH though, and have v 5.0.100 of the .Net SDK installed.

I figured somebody had broken the tests in the master branch and that the dev container was still a work in progress. Maybe that's a bad assumption though?

That said, I do believe you're right though that I fixed the wrong code. Although, I'm wondering if we actually want the fix applied to both places. I'll update the PR and let you comment on it. Thanks Tyler!

@deadlydog deadlydog force-pushed the 1393-AlwaysUseLocalHelpForFullHelp branch 2 times, most recently from b83e311 to 7925b3d Compare November 25, 2020 01:46
@TylerLeonhardt
Copy link
Member

The easiest way to test your change manually is by using this guide:
https://github.com/PowerShell/vscode-powershell/blob/master/docs/development.md

That will start a local build of the vscode extension with your local build of PSES.

@deadlydog
Copy link
Contributor Author

deadlydog commented Nov 25, 2020

Awesome, that would have been useful. Once I followed the code from the vscode-powershell repo to this one, I never went back to it. I kind of assumed there'd be a stand-alone way to test this git repo; although since I was experiencing the problem in VS Code, in retrospect it seems logical to go back to that repo and see how to integrate my changes from this repo into it. Still, this would be useful info to have on the ReadMe 😉

@TylerLeonhardt
Copy link
Member

Still, this would be useful info to have on the ReadMe 😉

Done:
https://github.com/PowerShell/PowerShellEditorServices/blob/master/README.md#development

If the user has overridden the Get-Help -Online parameter to always be
true, then calls to Get-Help will always launch a new browser window
to the cmdlet help. Instead, we want it to return the full cmdlet text
as intended, otherwise things like intellisense and tooltips may end
up opening the cmdlet help in a new browser window.
@deadlydog deadlydog force-pushed the 1393-AlwaysUseLocalHelpForFullHelp branch from 7925b3d to 680c6d4 Compare November 25, 2020 05:04
@deadlydog
Copy link
Contributor Author

Following that doc I was able to test my changes :)

  1. I re-added $PSDefaultParameterValues.Add("Get-Help:Online", $true) to my PowerShell Profile
  2. Built the PowerShellEditorServices from the master branch
  3. Ran code --extensionDevelopmentPath="c:\path\to\vscode-powershell" to confirm I could reproduce the issue, both from intellisense and tooltips.
  4. Switched the PowerShellEditorServices to my branch and rebuilt
  5. Ran code --extensionDevelopmentPath="c:\path\to\vscode-powershell" again and confirmed that the intellisense and tooltips now show up properly without popping open a new browser window.

I also tested clicking on the Help icon in the PowerShell Command Explorer that you mentioned, but it seems it always open using the Online help, even when the -Online parameter is not defaulted to true. Since my original "fix" didn't seem to make any difference, I just removed it.

So this PR now has only the change that actually addresses the issue I reported, and I've confirmed it does indeed solve the problem.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM Given the side effect of this change, I can't think of any test that could be written with a reasonable amount of work so we can skip that here.

Will merge tomorrow!

@TylerLeonhardt TylerLeonhardt merged commit de81cc4 into PowerShell:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants