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

Improve implicit object call support #54

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

keidax
Copy link
Collaborator

@keidax keidax commented Dec 19, 2024

more supported syntax 🤝 smaller parser size

@keidax
Copy link
Collaborator Author

keidax commented Dec 19, 2024

implicit_object_call now has the same fields as call, and implicit_object_assign mirrors assign. I wonder if it makes sense to just rename them? What do you all think on the value of keeping implicit_object_* nodes as separate variants in the syntax tree?

@nobodywasishere
Copy link
Member

I'm fine aliasing implicit_obj_* to just calls / etc, as long as it doesn't break impl object in case statements

Copy link
Collaborator

@devnote-dev devnote-dev left a comment

Choose a reason for hiding this comment

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

Aliasing them sounds like the best move here.

We can reuse tokens for `$.instance_var` and others, instead of creating
separate token versions with the leading `.`

The fields for `implicit_object_call` tree nodes now match the fields
for `call` nodes

More implicit call syntax is supported
@keidax keidax marked this pull request as draft December 22, 2024 22:29
@keidax
Copy link
Collaborator Author

keidax commented Dec 22, 2024

There's a different direction I want to take this PR, but I'd like to get some clarity on crystal-lang/crystal#15303 before moving forward. Any changes to the block precedence rules could cause conflicts with this change.

@keidax keidax force-pushed the keidax/implicit-index branch from bd89120 to 79d349f Compare December 23, 2024 17:35
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.

3 participants