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

Improving handling of string templates in the Java syntax. #6745

Merged
merged 1 commit into from Nov 30, 2023

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 24, 2023

When String Templates (a Java preview feature) are used in the VS Code, the content may look very weird, like:

  • before semantic highlighting is applied: string-templates-original-before-semantic-highlighting
  • after semantic highlighting is applied: string-templates-original-after-semantic-highlighting

Note that e.g. the nested String is not highlighted as a String. The "in String" and "outside of String" is inverted for the nested expression, as the grammar does not understand that the content of \{...} is not a String, but an expression.

This patch attempts to resolve that, by nesting #code inside \{...}. But, the inside of \{...} should not have string.quoted.double.java scope, so the scope is removed from the pattern that matches the string, and is moved to all the components, except the one of the inside of \{...}.

With this patch, the highlighting (with semantic highlighting) looks like this:
string-templates-before-semantic-highlight

Which seems satisfactory to me.

@lahodaj lahodaj added the VSCode Extension [ci] enable VSCode Extension tests label Nov 24, 2023
@lahodaj lahodaj added this to the NB21 milestone Nov 24, 2023
@lahodaj lahodaj requested a review from dbalek November 24, 2023 15:31
Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

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

Looks fine.

@mbien
Copy link
Member

mbien commented Nov 24, 2023

somewhat related: JEP 430 also states: "We strongly encourage IDEs to visually distinguish a string template from a string literal, and a text block template from a text block."

I personally don't think this is necessary since too many colors just add noise, the unusual syntax is already a distinguishing factor, but I do expect some users wanting to configure the IDE to render them differently, alone due to the fact that it was mentioned in the JEP.

Currently they are just a String literal and can not be configured separately as far as I see.

@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 24, 2023

The JEPs says:
Within the content of a string template or text block template, IDEs should visually distinguish an embedded expression from literal text.

Which is what NB does, and what this patch does. Inside NB, we could highlight the String fragments differently based on lexer, but I don't think I see the need. For the VS Code grammar, I don't even know how to do it - we would need to adjust the scopes based on presence/absence of \{...}.

In both cases, we could create a semantic highlight key for string templates, but I think it is already visually distinct enough.

@mbien
Copy link
Member

mbien commented Nov 24, 2023

I don't disagree, I think the unique invocation syntax of FOO."template" is already enough of an distinguishing factor, and as soon as variables are used it should be obvious that it is not a regular String.

But the JEP is not 100% clear there I think. If it is only about the second sentence, the first sentence wouldn't have to mention string literals or text blocks at all, it would be sufficient to recommend to render variables differently as distinguishing factor.

@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 27, 2023

I was thinking of this for a bit, and also asked someone associated with the JEP (seems it was meant so that we highlighting the embedded expressions as expressions - which we do, and also the braces in a different way, which we don't). I think I would like to try with this as it is, and see how that works. We can adjust if/as needed. Please let me know if there are any objections. Thanks!

@mbien
Copy link
Member

mbien commented Nov 27, 2023

@lahodaj all good! It was just me reading the JEP wrong. I do actually like how it looks at the moment (as mentioned before). Thanks for asking your colleagues for clarification.

The default NB theme doesn't even distinguish char from string literals (which is fine) since the difference rarely matters:

image

seems it was meant so that (...), and also the braces in a different way, which we don't

highlighting \{ is probably easy since \ is an escape character so it could use existing code which highlights \n etc, finding the matching bracket would require new logic most likely. Btw I fixed a small bug in ml string brace matching: #6726

@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 30, 2023

FWIW, I like how it looks currently as well.

@lahodaj lahodaj merged commit 063d123 into apache:master Nov 30, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants