Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ExpressionLanguage] Add a way to hook on each node when dumping the AST#19060
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| $arg =newNode\ConstantNode($token->value); | ||
| $arg =newNode\NameNode($token->value); |
nicolas-grekasJun 15, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The AST is more correct when usingNameNode instead ofConstantNode for representingGetAttrNode::PROPERTY_CALL andGetAttrNode::METHOD_CALL (but keepConstantNodeforGetAttrNode::ARRAY_CALL).
This is enlighten by the "unescaping" (this trim) that was required otherwise when dumping aGetAttrNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I tend to agree that this semantically more correct. Is it a BC break? I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes it's a BC break. I createda melody script to reproduce it.
@fabpot it's because of this change your gist your sent me yesterday does not work anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Arf, any change is a BC break after all :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, we need to revert this change, that's two issues as of now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted now
| $tokens[] =','; | ||
| $tokens[] =$pair['value']; | ||
| } | ||
| array_shift($tokens); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
isn't it better to add the, token after each pair, and to pop the last one here ? It would avoid having to reindex the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
a5e5273 to82177f4Compare| publicfunctiondump() | ||
| { | ||
| $str =''; | ||
| $tokens =array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Token has another meaning in the component, so we need to use something else.
fabpot commentedJun 15, 2016
You should also add a phpdoc to the |
fabpot commentedJun 15, 2016
And I think that the |
nicolas-grekas commentedJun 15, 2016
Comments addressed, |
fabpot commentedJun 15, 2016
👍 |
fabpot commentedJun 15, 2016
Thank you@nicolas-grekas. |
…en dumping the AST (nicolas-grekas)This PR was merged into the 3.2-dev branch.Discussion----------[ExpressionLanguage] Add a way to hook on each node when dumping the AST| Q | A| ------------- | ---| Branch? | "master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19013| License | MIT| Doc PR | -This is an iteration over#19013 to allow writing dumpers that can decorate dumps (e.g. add HTML tags based on each node type to do syntax highlighting).Commits-------66d23db [ExpressionLanguage] Add a way to hook on each node when dumping the AST
This PR was merged into the 3.2-dev branch.Discussion----------[ExpressionLanguage] fixed a BC break| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | fixes a BC break :)| Deprecations? | no| Tests pass? | yes| Fixed tickets | see#19060 (comment)| License | MIT| Doc PR | n/aCommits-------b00930f [ExpressionLanguage] fixed a BC break
This is an iteration over#19013 to allow writing dumpers that can decorate dumps (e.g. add HTML tags based on each node type to do syntax highlighting).