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 syntax highlighting after imports with instantiation arguments #2792

Merged
merged 5 commits into from
Oct 9, 2023

Conversation

ginsbach
Copy link
Contributor

@ginsbach ginsbach commented Sep 8, 2023

Fixes https://github.com/github/codeql-core/issues/3697

This PR updates the TextMate grammar to allow instantiation arguments in import statements. This will improve syntax highlighting. Specifically, It is meant to fix the highlighting of pragma in the example below.

image

Note that I don't know the TextMate grammar world very well, so am not really sure whether what I did here makes any sense.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@ginsbach ginsbach marked this pull request as ready for review September 8, 2023 09:06
@ginsbach ginsbach requested a review from a team as a code owner September 8, 2023 09:06
@robertbrignull
Copy link
Contributor

robertbrignull commented Sep 8, 2023

Sorry I don't think I'm qualified to review this properly, so hopefully someone with more experience of this area can help.

I did notice that the following files were accidentally committed. Could you remove them please

  • extensions/.DS_Store
  • extensions/ql-vscode/.DS_Store
  • extensions/ql-vscode/test/.DS_Store
  • extensions/ql-vscode/test/common/.DS_Store

@ginsbach ginsbach force-pushed the ginsbach/TextMateInstantiationSyntax branch from 030248e to 8f37b90 Compare September 8, 2023 13:02
Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks for the edits! Looks good from an admin point-of-view, but it would be great to get a look from someone who knows about textmate 😅

@dbartol
Copy link
Contributor

dbartol commented Sep 25, 2023

This seems to be fixing the highlighting of the #pragma, but I don't think it's actually highlighting the instantiation as part of the import-directive node. I think I've got an idea of how to fix it differently. I'll propose some changes in a bit.

Protip: The VS Code command Developer: Inspect Editor Tokens and Scopes lets you see exactly how the grammar tagged the character at the current cursor position.

@dbartol
Copy link
Contributor

dbartol commented Sep 25, 2023

@ginsbach I just opened #2859, which targets your PR branch, with a suggested alternate fix.

@ginsbach ginsbach force-pushed the ginsbach/TextMateInstantiationSyntax branch from 8f37b90 to 22a5661 Compare October 6, 2023 11:42
@ginsbach
Copy link
Contributor Author

ginsbach commented Oct 6, 2023

@ginsbach I just opened #2859, which targets your PR branch, with a suggested alternate fix.

Thank you! I have added the relevant commit to this PR and rebased on main to resolve the changelog conflict.

@ginsbach ginsbach requested review from dbartol and removed request for dbartol October 9, 2023 08:04
Copy link
Contributor

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM

@ginsbach ginsbach force-pushed the ginsbach/TextMateInstantiationSyntax branch from 22a5661 to 47fa163 Compare October 9, 2023 15:58
@ginsbach
Copy link
Contributor Author

ginsbach commented Oct 9, 2023

I have rebased on main to remove the merge conflict in the changelog.

@ginsbach ginsbach merged commit 86b2157 into main Oct 9, 2023
14 checks passed
@ginsbach ginsbach deleted the ginsbach/TextMateInstantiationSyntax branch October 9, 2023 16:17
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