-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Update GDScript highlighting for Godot 4 #3901
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
base: master
Are you sure you want to change the base?
Conversation
|
I would rather keep old keywords around because someone could be editing code written for an old Godot version. |
|
Finally got around to fixing this. I
|
Andriamanitra
left a comment
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 tested with two GDScript files I found on Github:
Triple quoted strings are missing handling for escaped characters, and I think it would make sense to make triple quoted strings into strings rather than comments (the documentation no longer mentions triple quoted strings as a way to create comments).
Other than that everything seems to look good.
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.
The triple quoted string highlighting actually needs to be above single quoted strings or the highlighting will see """x""" as three single quoted strings rather than one triple quoted string. 1 Check with syntax_test_gdscript.gd (it's easier to see what's going on while testing if you temporarily make triple quoted strings different color). You probably also need skip: "\\\\." to pass this test case:
var x = """ \""" """
var y = 123Footnotes
-
confusingly this is different than the situation we had earlier where the later rule was being applied on top of an earlier rule, that won't happen with groups with
start/end. ↩
|
I think this should fix that? |
runtime/syntax/gdscript.yaml
Outdated
| - constant: "\\b(INF|NAN|PI|TAU)\\b" | ||
| - constant.bool: "\\b(null|true|false)\\b" | ||
| # Function calls | ||
| - identifier: "[a-zA-Z_0-9]+\\(" |
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.
Can a function start with a number?
I assume that it is intentional to highlight the opening bracket, but not the closing, right?
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.
The bracket gets its color from statement later anyway so it doesn't matter. It's there just to limit the highlighting to function calls.
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.
Yep, the opening bracket just gets rehighlighted elsewhere. I couldn't figure out how to get a lookahead working so that was my next best option. I've fixed the function highlighting to check for word boundaries and handle numbers properly
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.
But that is not more than an unintended hack.
This should stop working after the highlighter is reworked, in which it builds kind of a tree.
Unless we improve the skip in that way in which it really was intended to work (provide a sort of a look ahead/around).
BTW:
Missed this one - identifier: "func\\s+[a-zA-Z_0-9]+"?
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.
But that is not more than an unintended hack.
This should stop working after the highlighter is reworked, in which it builds kind of a tree.
Unless we improve the skip in that way in which it really was intended to work (provide a sort of a look ahead/around).
I'm pretty sure it's an intended hack. It is already in use in other syntaxes 1, some of which have been using it ever since the current highlighting system was introduced in 2fcb40d.
Footnotes
-
Grepping for
\\("found kvlang, Lua, and PHP, and I'm pretty sure there are others that use the same trick for something slightly different ↩
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.
Well, okay...doesn't make the intended rework easier, when rules are intended to overlap. 🤔
Dirty thing...
| - constant: "\\b(INF|NAN|PI|TAU)\\b" | ||
| - constant.bool: "\\b(null|true|false)\\b" | ||
| # Function calls | ||
| - identifier: "\\b[a-zA-Z_][a-zA-Z_0-9]+\\(" |
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.
Not * as in line 21 with [a-zA-Z_][a-zA-Z_0-9]*?
It should follow the same rule for the function name, at least I'd expect.
This removes
exportand related keywords from the list of keywords and adds a new rule for highlighting annotations