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

Support for trailing comma #179

Merged
merged 25 commits into from
Apr 2, 2024
Merged

Conversation

vitorpy
Copy link
Contributor

@vitorpy vitorpy commented Mar 20, 2024

Closes #28.
Based on abandoned PR #49.

@anton-trunov
Copy link
Member

Hi @vitorpy, thanks for reviving that PR! You made the PR to be a draft, any additional things need to be done here?

@anton-trunov
Copy link
Member

btw, you probably want to remove src/test/features/serialization-2.tact from your PR -- it's just some unrelated whitespace changes

@vitorpy
Copy link
Contributor Author

vitorpy commented Mar 20, 2024

Hi @vitorpy, thanks for reviving that PR! You made the PR to be a draft, any additional things need to be done here?

Hey, I'm going to add support for trailing commas on function arguments lists as well.

@vitorpy vitorpy marked this pull request as ready for review March 20, 2024 14:50
@anton-trunov anton-trunov changed the title Includes support for trailing comma Support for trailing comma Mar 20, 2024
@anton-trunov anton-trunov self-assigned this Mar 20, 2024
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

I left a few suggestions that should be pretty easy to fix. Looking forward to having the trailing comma feature in the language!

src/test/features/serialization-2.tact Outdated Show resolved Hide resolved
src/grammar/test/case-25.tact Show resolved Hide resolved
src/grammar/grammar.ohm Outdated Show resolved Hide resolved
src/grammar/grammar.ohm Outdated Show resolved Hide resolved
src/grammar/grammar.ohm Outdated Show resolved Hide resolved
@anton-trunov
Copy link
Member

Please add also the corresponding entry in CHANGELOG.md (the Changed section)

@anton-trunov anton-trunov added the kind: language feature Intent to add a language feature label Mar 20, 2024
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Thanks for all the quick fixes. Just one tiny change and we are good to go

CHANGELOG.md Outdated Show resolved Hide resolved
src/grammar/test/case-25.tact Show resolved Hide resolved
@anton-trunov anton-trunov added this to the v1.3.0 milestone Mar 20, 2024
@anton-trunov
Copy link
Member

I just added this PR to the v1.3.0 milestone so we will make it available in the very next Tact release

@vitorpy
Copy link
Contributor Author

vitorpy commented Mar 20, 2024

I just added this PR to the v1.3.0 milestone so we will make it available in the very next Tact release

Thanks! That was fun! I'll see if I grab another issue 😃

@anton-trunov
Copy link
Member

Thanks! That was fun! I'll see if I grab another issue 😃

@vitorpy By all means! If you decide to do it, it would be nice if you outlined your solution before going for it, just to make sure we are on the same page and to make the code review process a faster and smoother experience.

@vitorpy
Copy link
Contributor Author

vitorpy commented Mar 21, 2024

Thanks! That was fun! I'll see if I grab another issue 😃

@vitorpy By all means! If you decide to do it, it would be nice if you outlined your solution before going for it, just to make sure we are on the same page and to make the code review process a faster and smoother experience.

Will do. This one I went directly as I saw the old PR and it seemed like a good way to get to know the code base a little without too much trouble. Fixed the test case - thanks for the review!

src/grammar/grammar.ohm Outdated Show resolved Hide resolved
src/grammar/test/case-25.tact Show resolved Hide resolved
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

CI shows that some tests are failing

You can see here how to run it locally:
https://github.com/tact-lang/tact/blob/main/.github/workflows/tact.yml#L35-L38

src/grammar/test-failed/case-19.tact Outdated Show resolved Hide resolved
src/grammar/test/case-17.tact Outdated Show resolved Hide resolved
@anton-trunov
Copy link
Member

Sorry for all the back and forth, I think I finally got the hang of writing and debugging tests for tact. Thanks!

Hey @vitorpy, no worries at all! Thank you for bringing this feature to Tact and helping to make the implementation more robust

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

CI is red unfortunately

src/grammar/test-failed/case-24.tact Outdated Show resolved Hide resolved
src/grammar/test-failed/case-23.tact Outdated Show resolved Hide resolved
@vitorpy
Copy link
Contributor Author

vitorpy commented Mar 31, 2024

@anton-trunov I'm kinda stuck on why case-0 is not throwing. Could you give me some pointers on how to progress?

src/grammar/grammar.ts Outdated Show resolved Hide resolved
anton-trunov
anton-trunov previously approved these changes Apr 2, 2024
src/grammar/test-failed/case-26.tact Outdated Show resolved Hide resolved
src/grammar/test-failed/case-25.tact Outdated Show resolved Hide resolved
anton-trunov
anton-trunov previously approved these changes Apr 2, 2024
@anton-trunov anton-trunov merged commit 716a93d into tact-lang:main Apr 2, 2024
3 checks passed
@anton-trunov
Copy link
Member

@vitorpy Thank you for your contribution! This qualifies for the Tact contributor SBT: https://society.ton.org/contribute-to-tact-compiler. I'll just need your TON wallet address to issue the token

@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 2, 2024

Awesome! I'll try to keep the contributions going.

UQAz6vmh0aKPSH5dJHsfHI8YQH55oLeuWMz4eQkr4ZjS-aG6

@vitorpy vitorpy deleted the trailing-comma branch April 2, 2024 19:48
@anton-trunov
Copy link
Member

SBT issued! Hope to see more things from you 👍

@verytactical verytactical added scope: parser Implementation of parser (src/grammar) and removed syntax kind: language feature Intent to add a language feature labels Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: parser Implementation of parser (src/grammar)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing comma
3 participants