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

allow brackets in expression_value nodes #5

Merged
merged 2 commits into from
Oct 19, 2021
Merged

allow brackets in expression_value nodes #5

merged 2 commits into from
Oct 19, 2021

Conversation

the-mikedavis
Copy link
Member

I found some odd behavior parsing a case like this:

<div phx-value-target={ "##{@id}" }></div>

where the expression_value node has a hard time dealing with the brackets {..}. Here's the parse result:

(fragment [0, 0] - [1, 0]
  (tag [0, 0] - [0, 42]
    (start_tag [0, 0] - [0, 36]
      (tag_name [0, 1] - [0, 4])
      (attribute [0, 5] - [0, 35]
        (attribute_name [0, 5] - [0, 21])
        (expression [0, 22] - [0, 35]
          (ERROR [0, 23] - [0, 27]
            (expression_value [0, 23] - [0, 27]))
          (expression_value [0, 27] - [0, 32])
          (ERROR [0, 32] - [0, 33]))))
    (end_tag [0, 36] - [0, 42]
      (tag_name [0, 38] - [0, 41]))))

I shuffled around the grammar rules for expression_value a bit so we use a hidden $._expression_value which can repeat to capture everything in the expression's brackets. Then we alias those repeating hidden nodes all together as one expression_value node. The tree for that example case ends up looking like so

(fragment [0, 0] - [1, 0]
  (tag [0, 0] - [0, 42]
    (start_tag [0, 0] - [0, 36]
      (tag_name [0, 1] - [0, 4])
      (attribute [0, 5] - [0, 35]
        (attribute_name [0, 5] - [0, 21])
        (expression [0, 22] - [0, 35]
          (expression_value [0, 23] - [0, 34]))))
    (end_tag [0, 36] - [0, 42]
      (tag_name [0, 38] - [0, 41]))))

@connorlay connorlay self-requested a review October 17, 2021 16:11
@connorlay
Copy link
Member

Hi @the-mikedavis , thanks for the bug report and PR! I'm going to pull this down and test it this evening. While I am doing that, could you add a test for this edge-cased?

@the-mikedavis
Copy link
Member Author

ah whoops I always forget about the test suite 😅

on it!

@connorlay
Copy link
Member

@the-mikedavis Thanks for the tests, this looks great to me.

Copy link
Member

@connorlay connorlay left a comment

Choose a reason for hiding this comment

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

🚢

Comment on lines +103 to +108
prec.left(
seq(
alias(repeat($._expression_value), $.expression_value),
'}'
)
)
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach a lot! I'll review the Surface grammar and see if it has the same bug or not. If so I'll port this fix over.

@connorlay connorlay merged commit c15baf4 into phoenixframework:main Oct 19, 2021
@the-mikedavis the-mikedavis deleted the expression-value-allow-brackets branch October 19, 2021 02:15
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.

2 participants