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

fix(kotlin): classes with newlines #476

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Conversation

brandonspark
Copy link
Contributor

@brandonspark brandonspark commented Mar 26, 2024

What:

This PR makes it so we can parse Kotlin classes that have a newline between the class identifier and constructor.

Why:

Parse rate.

How:

The problem is in cases like:

class Foo
constructor Bar() { ... }

Here, we insert an automatic semicolon between Foo and constructor. This leaves us able to parse class Foo as a class_declaration, but constructor Bar ... is not allowed on its own.

We simply allow constructor Bar ... to be a standalone statement, and resolve to stitch them together at parsing time.

Testing

Tested with make test and test-lang kotlin

Security

  • Change has no security implications (otherwise, ping the security team)

@brandonspark brandonspark requested a review from a team as a code owner March 26, 2024 17:16
@brandonspark brandonspark requested review from aryx and mjambon and removed request for a team March 26, 2024 17:16
@amchiclet
Copy link
Contributor

FYI make lang in Circle CI runs out of memory when parsing C sharp. Ran it locally, and it passed.

@amchiclet amchiclet requested a review from nmote March 27, 2024 18:38
Copy link
Collaborator

@nmote nmote left a comment

Choose a reason for hiding this comment

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

Not a fan of all the formatting changes in the tests. I guess the "hide whitespace" setting only has to do with whitespace changes on a single line, so to review, one still has to compare these manually.

I spot-checked some and they looked the same, but I'm assuming with this review that the only behavior change is for the added test cases.

@amchiclet
Copy link
Contributor

Also spot checked and arrived at the same conclusion. The only behavior change is the new test cases.

Thanks for the quick review!

@brandonspark
Copy link
Contributor Author

Not a fan of all the formatting changes in the tests. I guess the "hide whitespace" setting only has to do with whitespace changes on a single line, so to review, one still has to compare these manually.

I spot-checked some and they looked the same, but I'm assuming with this review that the only behavior change is for the added test cases.

the formatting changes are automatically generated from tree-sitter test -u. it's a pain, but this slight annoyance in review now will make future reviews easier.

@amchiclet amchiclet merged commit 878e710 into main Mar 27, 2024
2 of 3 checks passed
@amchiclet amchiclet deleted the brandon/kotlin-class-newlines branch March 27, 2024 18:56
@nmote
Copy link
Collaborator

nmote commented Mar 27, 2024

In the future, I'd prefer reformatting to be split into a separate PR

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

Successfully merging this pull request may close these issues.

3 participants