Skip to content

Parse it default parameter#2124

Merged
kddnewton merged 5 commits intoruby:mainfrom
k0kubun:it
Jan 17, 2024
Merged

Parse it default parameter#2124
kddnewton merged 5 commits intoruby:mainfrom
k0kubun:it

Conversation

@k0kubun
Copy link
Copy Markdown
Member

@k0kubun k0kubun commented Jan 2, 2024

close #2052

Copy link
Copy Markdown
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

One small change and then great! Thanks!

Comment thread src/prism.c Outdated
Copy link
Copy Markdown
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I think this might be more complicated than this unfortunately.

If you get it {} I think it needs to change back into a method call. This is also the case for when it receivers arguments without parentheses.

@k0kubun
Copy link
Copy Markdown
Member Author

k0kubun commented Jan 2, 2024

I tried some of such cases with local Prism.parse, and they seemed working. Though I guess I'm still missing something for passing lex-ruby/discourse/top-100, so I'll look into them.

@k0kubun k0kubun marked this pull request as draft January 3, 2024 05:02
Comment thread src/prism.c Outdated
@k0kubun k0kubun dismissed kddnewton’s stale review January 12, 2024 22:16

We've fixed all cases except for pattern matching => ^it. Do we want to address it in this PR, or could that be a separate PR?

Copy link
Copy Markdown
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I think we should add => ^it in this PR. Also we will need to document that the local variable read nodes for it will actually use 0it for the constant name, since that's pretty weird.

@k0kubun k0kubun dismissed kddnewton’s stale review January 17, 2024 06:07

Fixed those two things 313be8e 24a2872. I think that's it?

Comment thread src/prism.c Outdated
Copy link
Copy Markdown
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Could you add a test ensuring that with version 3_3_0 (Prism.parse(code, version: ...)) { it } is parsed as a method call?
That seems good to add and should help ensure the right checks are in place.

k0kubun and others added 2 commits January 17, 2024 09:08
Co-authored-by: Kevin Newton <kddnewton@gmail.com>
@k0kubun k0kubun dismissed kddnewton’s stale review January 17, 2024 17:24

addressed the feedback in 9778377 and 94ecb36.

Copy link
Copy Markdown
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Great job @k0kubun!

@kddnewton kddnewton merged commit 2489139 into ruby:main Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support it default parameter

3 participants