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

Basic multiline highlighting #13

Merged
merged 30 commits into from
Aug 23, 2024
Merged

Basic multiline highlighting #13

merged 30 commits into from
Aug 23, 2024

Conversation

sungshik
Copy link
Collaborator

@sungshik sungshik commented Aug 20, 2024

This PR adds basic support for multiline highlighting:

  • The analysis stage of the conversion has remained roughly the same (i.e., most information was already available).
  • Most changes are in the transformation stage, which now cleanly separates the generation of "inner rules" (detect the internal content of a production) and "outer rules" (detect the external context of a production). See the new module ConversionUnit for more details in the comments.

All tests are updated accordingly as well (and some new ones were added).

…h the latest version of the semantic tokenizer
…nd inner/outer rules

Note: This changes the nature of conversion units from types to only read from, to types to also write to.
Copy link
Collaborator Author

@sungshik sungshik left a comment

Choose a reason for hiding this comment

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

A few additional clarifying comments

@sungshik sungshik marked this pull request as ready for review August 21, 2024 11:21
Copy link

@toinehartman toinehartman left a comment

Choose a reason for hiding this comment

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

That's a lot of work again! I like the readability of the code and the usage of a lot of utility functions. To me, this helps a lot with readability.
Good choice to split out some of the logic to separate modules.

The tests are hard to review for me, but at least look like they cover a lit of cases.

"name": "constant.regexp"
},
"2": {
"name": ""
Copy link
Member

Choose a reason for hiding this comment

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

@sungshik is this a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, but it's not particularly elegant either 😉 Some generated regexes have groups that are used only internally for matching, without a category. These show up here as empty strings. This has no impact on the highlighting, but I agree it would be nicer to not have these degenerate entries in the captures map at all. It requires the addition of just a simple non-emptiness check. I've fixed that now.

@sungshik
Copy link
Collaborator Author

Thank you, Toine, Davy, and Pieter for the comments 🙂

@sungshik sungshik merged commit 1a3b465 into main Aug 23, 2024
2 checks passed
@sungshik sungshik deleted the basic-multiline-highlighting branch August 23, 2024 10:44
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