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

Print (foo^)##bar with the right precedence #2044

Merged
merged 2 commits into from
Jul 4, 2018

Conversation

anmonteiro
Copy link
Member

fixes #1554

@IwanKaramazow
Copy link
Contributor

method simple_enough_to_be_lhs_dot_send ?(except_unary_postfix=false)
The except_unary_postfix name arg feels very specific/specialized. Can we pass the relevant expression down to make it a bit more generic/extendable in the future? There might be other cases.
I might be totally wrong though, this is just an idea.

@anmonteiro
Copy link
Member Author

anmonteiro commented Jul 2, 2018

I agree it feels too specialized. I'm definitely open to other ideas, but the point here is that in the presence of a SHARPOP we want to formatPrecedence of UnaryPostfix.

Maybe the optional argument is fine and we just need to change its name to sharpop?

@IwanKaramazow
Copy link
Contributor

random idea, what about passing ?infixOp as some kind of context?

@anmonteiro
Copy link
Member Author

@IwanKaramazow check my latest commit. I replaced except_unary_postfix with an infix_context that we can pattern match against.

I liked your idea, and I believe this might prove to be more extensible in the future if we wanna tweak other infix ops.

What do you think?

@IwanKaramazow
Copy link
Contributor

Feels like a nice step into the right direction :)
If you want to avoid the extra optional allocation, you can use the empty string as default value ?(except_unary_postfix=""). The current approach is fine, won't make much of a difference here.

@chenglou ready for me, merge approved.

@anmonteiro
Copy link
Member Author

I thought about the empty string but it didn't feel very nice to me. I'll change it if you'd like though

@IwanKaramazow
Copy link
Contributor

No, not necessary, option makes sense here

@chenglou
Copy link
Member

chenglou commented Jul 4, 2018

Thanks

@chenglou chenglou merged commit 6faf401 into reasonml:master Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard time parsing (curNode^)##childNodes and wrong refmt
4 participants