-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Update java highlight mode to support 21 version features #5343
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5343 +/- ##
==========================================
+ Coverage 87.41% 87.44% +0.02%
==========================================
Files 574 575 +1
Lines 45613 45679 +66
Branches 6935 6939 +4
==========================================
+ Hits 39873 39943 +70
+ Misses 5740 5736 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
src/mode/java_highlight_rules.js
Outdated
"strings": [ | ||
{ | ||
token: "string", | ||
regex: /(?:STR|FMT)\."/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct. Technically the prefix can be any expression so long as it returns a TemplateProcessor
so
try (var conn = ds.getConnection()) {
var stmt = conn."""
SELECT * FROM table WHERE x = \{x}
""";
}
or
a.b().c().e()."""
Sub \{here}
"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, new approach should work. The only problem I see - is usage of outstanding ."""text""" without adding any processor, but for solving this case mode should recognize whole contexts and this would be a little bit too much.
I see i was requested to review it - give me a minute to figure out how to test it locally... |
Hey @bowbahdoe thank you for the review! Would you consider the issue you highlighted above a deal breaker to merge this PR? I'm not a Java user myself so it's hard for me to evaluate the severity of the issue you pointed out above. @mkslanc please take a look at the issue above, maybe fixing it would be faster than discussing it :) |
@bowbahdoe @InspiredGuy This change should make it :) |
@mkslanc I will take a look tonight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good. Once there is a release, i'll update https://java-playground.com and if we find any more issues i'll report them
Thank you @bowbahdoe , your help is appreciated! |
Issue #, if available: #5338 #2742
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.