Skip to content

Updated Semantic Handler to work with new LSP #1343

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 5 commits into from
Aug 7, 2020

Conversation

justinytchen
Copy link
Contributor

@justinytchen justinytchen commented Aug 4, 2020

PsesSemanticTokensHandler was not compatible with the Omnisharp.Extensions.LanguageServer update to 0.17.3 following the new LSP spect (v3.18). This PR fixes those issues.

Also see: PowerShell/vscode-powershell#2861

@justinytchen justinytchen requested a review from rjmholt as a code owner August 4, 2020 18:57
@@ -1162,7 +1162,7 @@ await PsesLanguageClient
var expectedArr = new int[5]
{
// line, index, token length, token type, token modifiers
0, 0, scriptContent.Length, 2, 0 //function token: line 0, index 0, length, type 2 = keyword, no modifiers
0, 0, 8, 1, 0 //function token: line 0, index 0, length 8, type 1 = keyword, no modifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 8 not the length of the script content here?

@@ -91,7 +91,7 @@ private async Task CheckPackageManagement()

if (_powerShellContextService.CurrentRunspace.Runspace.SessionStateProxy.LanguageMode != PSLanguageMode.FullLanguage)
{
_languageServer.Window.ShowWarning("You have an older version of PackageManagement known to cause issues with the PowerShell extension. Please run the following command in a new Windows PowerShell session and then restart the PowerShell extension: `Install-Module PackageManagement -Force -AllowClobber -MinimumVersion 1.4.6`");
_languageServer.Window.LogWarning("You have an older version of PackageManagement known to cause issues with the PowerShell extension. Please run the following command in a new Windows PowerShell session and then restart the PowerShell extension: `Install-Module PackageManagement -Force -AllowClobber -MinimumVersion 1.4.6`");
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right... the spec says there's a ShowMessage and a LogMessage...

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#window_showMessage

@david-driscoll did this API change?

Choose a reason for hiding this comment

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

hmm... not intentionally.

Choose a reason for hiding this comment

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

taking a look at the assembly.

Choose a reason for hiding this comment

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

yep, I deleted one to many methods!

Choose a reason for hiding this comment

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

I will bring those back and get 0.17.4 out to fix that.

Choose a reason for hiding this comment

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

as a work around for the moment you can use ShowMessage and give it the correct message type.

Choose a reason for hiding this comment

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

New package is out, this should restore that function.
https://www.nuget.org/packages/OmniSharp.Extensions.LanguageServer/0.17.4

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.

tiny nit

…esSemanticTokensHandler.cs

Co-authored-by: Tyler James Leonhardt <[email protected]>
@TylerLeonhardt TylerLeonhardt merged commit 2fcadd8 into PowerShell:master Aug 7, 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
Development

Successfully merging this pull request may close these issues.

4 participants