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

Add textmate grammar #2069

Merged
merged 10 commits into from
Apr 1, 2021
Merged

Add textmate grammar #2069

merged 10 commits into from
Apr 1, 2021

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Mar 30, 2021

Related to #596
Related to #2098

Preview the highlighting using GitHub Linguist here

Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

This is awesome!

}
}]

resource secrets4 'Microsoft.KeyVault/vaults/secrets@2018-02-14' = [for secret in secretsObject.secrets: if (true) {
Copy link
Member

@majastrz majastrz Mar 31, 2021

Choose a reason for hiding this comment

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

secret [](start = 74, length = 6)

I think it'd be worth highlighting limitations of TM in these tests as well. For example it'd be ok if for was treated as a keyword even if it was also the name of the loop item variable. (And equivalent clashes elsewhere in the grammar as well.)

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

:shipit:

@anthony-c-martin anthony-c-martin enabled auto-merge (squash) April 1, 2021 18:50
@majastrz
Copy link
Member

majastrz commented Apr 1, 2021

This is great!

One question we need to answer at some point is whether we should include the TM grammar in the VSIX. I think it'd be neat to have coloring appear immediately while the server loads, but it makes it harder for users to notice semantic highlights issues. Maybe we shouldn't do that until we have an experience for detecting and easily enabling semantic highlights in config to work around theme issues?

auto-merge was automatically disabled April 1, 2021 18:58

Head branch was pushed to by a user without write access

@anthony-c-martin
Copy link
Member Author

This is great!

One question we need to answer at some point is whether we should include the TM grammar in the VSIX. I think it'd be neat to have coloring appear immediately while the server loads, but it makes it harder for users to notice semantic highlights issues. Maybe we shouldn't do that until we have an experience for detecting and easily enabling semantic highlights in config to work around theme issues?

Personally, I'd only want to include it in the VSIX for hovers, and possibly for use in markdown fenced code blocks. I think we'd need to make the TM grammar significantly more robust (inc full error recovery) to provide highlighting before semantic tokens kick in.

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.

3 participants