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

Added Handler for Semantic Tokenization #1328

Merged
merged 64 commits into from
Jul 30, 2020

Conversation

justinytchen
Copy link
Contributor

@justinytchen justinytchen commented Jul 16, 2020

Description
Fixes PowerShell/vscode-powershell#2813

Added a handler that utilizes the Parser in System.Management.Automation.Language to get PowerShell tokens and convert them to the semantic tokens that VS Code expects.

To use this, add "semanticHighlighting": true in the theme.json file.
Make sure a call is made to languageServerClient.registerProposedFeatures() in session.ts (vscode-powershell)
See PowerShell/vscode-powershell#2834

Before:
image

After:
image

Some issues this helps address:
PowerShell/EditorSyntax#143
PowerShell/EditorSyntax#157
PowerShell/EditorSyntax#167
PowerShell/EditorSyntax#191
PowerShell/EditorSyntax#198
PowerShell/EditorSyntax#199
PowerShell/EditorSyntax#200
PowerShell/EditorSyntax#201

@justinytchen justinytchen requested a review from rjmholt as a code owner July 16, 2020 23:11
@rjmholt
Copy link
Contributor

rjmholt commented Jul 17, 2020

Given that this is quite a nice, standalone service, we probably should add some e2e tests for this

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.

Looking good - I had a few more comments. Don't forget that curly braces start on their own line:

public void foo()
{
}

instead of

public void foo() {
}

Also gave a bit of context in @rjmholt's comments

log20200713.txt Outdated Show resolved Hide resolved

namespace Microsoft.PowerShell.EditorServices.Handlers
{
//Disable warnings having to do with SemanticTokensHandler being labelled obsolete
//SemanticTokensHandler is labeled "Obsolete" because that is how Omnisharp marks proposed LSP features. Since we want this proposed feature, we disable this warning.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//SemanticTokensHandler is labeled "Obsolete" because that is how Omnisharp marks proposed LSP features. Since we want this proposed feature, we disable this warning.
// SemanticTokensHandler is labeled "Obsolete" because that is how Omnisharp marks proposed LSP features. Since we want this proposed feature, we disable this warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

A question here: we already compile with warnings today, and while that's not a good thing, this particular warning should eventually disappear.

If we keep the pragma we suppress a warning that should eventually either go away, or which we should pay attention to since it will indicate that the proposed API hasn't stabilised. When it is stabilised, we would need to remove the pragma, but we won't know because of the pragma, meaning it may hang around in the code indefinitely (like so many fxcop suppressions).

Instead I think it probably makes sense for us to see the warning and not use a suppression pragma


namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class PsesSemanticTokens : SemanticTokensHandler
Copy link
Contributor

@rjmholt rjmholt Jul 21, 2020

Choose a reason for hiding this comment

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

Suggested change
internal class PsesSemanticTokens : SemanticTokensHandler
internal class PsesSemanticTokensHandler : SemanticTokensHandler

Copy link
Member

Choose a reason for hiding this comment

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

Tokens*

Copy link
Contributor

Choose a reason for hiding this comment

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

(Fixed in my suggestion)

Comment on lines +8 to +15
public SemanticToken(string text, SemanticTokenType type, int line, int column, IEnumerable<string> tokenModifiers)
{
Line = line;
Text = text;
Column = column;
Type = type;
TokenModifiers = tokenModifiers;
}
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but just wanted to say that if you get rid of this ctor, you can do:

new SemanticToken
{
    Text = "asdf",
    Line = 1,
    Column = 2,
   ...
}

and that will work the same way.

We can defer that for now.

@@ -0,0 +1,201 @@
[
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

@TylerLeonhardt TylerLeonhardt merged commit 6095ae5 into PowerShell:master Jul 30, 2020
@david-driscoll
Copy link

🥳

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.

Semantic Highlighting for Powershell in VS Code
4 participants