Skip to content

Parser: give syntax error on comma after newline in argument list#6514

Merged
RX14 merged 1 commit into
crystal-lang:masterfrom
asterite:bug/6513-syntax-error-comma-after-newline
Aug 29, 2018
Merged

Parser: give syntax error on comma after newline in argument list#6514
RX14 merged 1 commit into
crystal-lang:masterfrom
asterite:bug/6513-syntax-error-comma-after-newline

Conversation

@asterite
Copy link
Copy Markdown
Member

@asterite asterite commented Aug 9, 2018

Fixes #6513

Code like this:

foo(x
, y)

currently doesn't give a syntax error, but it should (like in Ruby). We can of course discuss whether it should really be a syntax error or not. My reasoning is that the comma signals that more arguments are coming, and it's confusing that the comma starts the next line. For example, if we remove the parentheses:

foo x
, y

that won't compile. So code with and without parentheses should look more or less the same (maybe this is a weaker argument, but still...).

I believe in many other languages this is a syntax error too.

This of course is a breaking change, but I doubt there are many out there that write code like this.

This syntax option was available for many other things, like method arguments, but not available in some other things, like array literals. Now it's unified to give a syntax error.

@asterite
Copy link
Copy Markdown
Member Author

asterite commented Aug 9, 2018

Hmmm... actually, Java, C# and Python allow such syntax. But Ruby and Go don't. So I guess it's a matter of preference.

Right now it's behaving in a way or another depending on the case, so it's not unified. Let's use this PR to discuss what should we do with this. Maybe this PR is fine, it hurts my eyes a bit to see that stray comma there :-P

@asterite asterite force-pushed the bug/6513-syntax-error-comma-after-newline branch from 759d686 to d9bdf9e Compare August 9, 2018 23:23
@asterite asterite force-pushed the bug/6513-syntax-error-comma-after-newline branch from d9bdf9e to fe6c135 Compare August 9, 2018 23:56
@hugoabonizio
Copy link
Copy Markdown
Contributor

This syntax (leading comma) was used in JS community, but isn't used widely nowadays. I guess everybody realised it's ugly.

@RX14 RX14 added topic:compiler breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. labels Aug 11, 2018
Copy link
Copy Markdown
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Judging by the silence on this issue, this is fine to remove

Copy link
Copy Markdown
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

I believe this is a good change which makes the syntax easier to understand and more uniform. Let's go for it 👍
Thank you @asterite 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. kind:feature topic:compiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants