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.

Sign up for GitHub

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

Jump to bottom

GDScript: Fix multiline and trailing comma for assert #70655

Merged
merged 1 commit into from
Jan 9, 2023
Merged

GDScript: Fix multiline and trailing comma for assert #70655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,24 +1799,29 @@ GDScriptParser::AssertNode *GDScriptParser::parse_assert() {
// TODO: Add assert message.
AssertNode *assert = alloc_node<AssertNode>();

push_multiline(true);
consume(GDScriptTokenizer::Token::PARENTHESIS_OPEN, R"(Expected "(" after "assert".)");

assert->condition = parse_expression(false);
if (assert->condition == nullptr) {
push_error("Expected expression to assert.");
pop_multiline();
complete_extents(assert);
return nullptr;
vonagam marked this conversation as resolved.
Show resolved Hide resolved
}

if (match(GDScriptTokenizer::Token::COMMA)) {
// Error message.
if (match(GDScriptTokenizer::Token::COMMA) && !check(GDScriptTokenizer::Token::PARENTHESIS_CLOSE)) {
assert->message = parse_expression(false);
if (assert->message == nullptr) {
push_error(R"(Expected error message for assert after ",".)");
pop_multiline();
complete_extents(assert);
return nullptr;
vonagam marked this conversation as resolved.
Show resolved Hide resolved
}
match(GDScriptTokenizer::Token::COMMA);
}

pop_multiline();
consume(GDScriptTokenizer::Token::PARENTHESIS_CLOSE, R"*(Expected ")" after assert expression.)*");

complete_extents(assert);
Expand Down
24 changes: 24 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/multiline_assert.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
func test():
var x := 5

assert(x > 0)
assert(x > 0,)
assert(x > 0, 'message')
assert(x > 0, 'message',)

assert(
x > 0
)
assert(
x > 0,
)
assert(
x > 0,
'message'
)
assert(
x > 0,
'message',
)
Comment on lines +5 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Why are the trailing commas allowed? It doesn't make any sense to me, I'd expect an error when there's a trailing comma.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, the trailing comma doesn't really make sense.

Copy link
Contributor Author

@vonagam vonagam Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand the surprise:

func method(a, b = 'c',) -> void:
  print(a, b,)

func _ready() -> void:
  method('a',)
  method('a', 'b',)
  print(['a',],)

Trailing commas and multi-line are supported by method calls and arrays. That's just make it behave consistent with those. What am I missing?

Copy link
Member

Choose a reason for hiding this comment

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

@vnen Is this a bug or a feature?

Copy link
Contributor Author

@vonagam vonagam Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is a feature. Search for "// Allow for trailing comma." comment in parser - parse_call, parse_signal, parse_enum, parse_function_signature, parse_array, parse_dictionary all have that comment for code that allows trailing comma.

The utility is visual consistency and smaller more relevant diffs/blame in git. So for example, you can write code like:

assert(
  condition,
)

And then later in a separate commit add an error message, that would be 1 line addition instead of addition of 2 lines and removal of 1.


print('OK')
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/parser/features/multiline_assert.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_OK
OK