[ExpressionLanguage] Add a way to hook on each node when dumping the AST #19060
Conversation
| @@ -330,7 +330,7 @@ public function parsePostfixExpression($node) | |||
| throw new SyntaxError('Expected name', $token->cursor); | |||
| } | |||
|
|
|||
| $arg = new Node\ConstantNode($token->value); | |||
| $arg = new Node\NameNode($token->value); | |||
There was a problem hiding this comment.
The AST is more correct when using NameNode instead of ConstantNode for representing GetAttrNode::PROPERTY_CALL and GetAttrNode::METHOD_CALL (but keep ConstantNodefor GetAttrNode::ARRAY_CALL).
This is enlighten by the "unescaping" (this trim) that was required otherwise when dumping a GetAttrNode.
There was a problem hiding this comment.
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.
Yes it's a BC break. I created a 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.
Arf, any change is a BC break after all :)
There was a problem hiding this comment.
So, we need to revert this change, that's two issues as of now.
| } | ||
| array_shift($tokens); |
There was a problem hiding this comment.
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
nicolas-grekas
force-pushed
the
es-dump
branch
2 times, most recently
from
a5e5273
to
82177f4
Compare
| @@ -27,12 +27,14 @@ public function compile(Compiler $compiler) | |||
|
|
|||
| public function dump() | |||
| { | |||
| $str = ''; | |||
| $tokens = array(); | |||
There was a problem hiding this comment.
Token has another meaning in the component, so we need to use something else.
|
You should also add a phpdoc to the |
|
And I think that the |
|
Comments addressed, |
|
👍 |
|
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/a Commits ------- 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).