[ExpressionLanguage] Added a way to dump the AST #19013
Conversation
| $str .= sprintf('%s, ', $pair['value']->dump()); | ||
| } | ||
|
|
||
| $str = rtrim($str, ', '); |
There was a problem hiding this comment.
you can return here directly (same everywhere)
|
Cannot be accepted without tests |
|
I addressed your comments, and I added some tests. |
|
|
||
| foreach ($this->getKeyValuePairs() as $pair) { | ||
| $key = $pair['key']->attributes['value']; | ||
| if (!is_int($key)) { |
There was a problem hiding this comment.
when int keys are used but not in numerical order, this won"t work, isn't it?
There was a problem hiding this comment.
I don't see why it would not work. but if it's the case, how could I fix it?
2dbc349 to
23a5c66
Compare
|
|
||
| protected function dumpEscape($value) | ||
| { | ||
| return str_replace(['\\', '"'], ['\\\\', '\"'], $value); |
There was a problem hiding this comment.
I fixed another one too, and added more tests. RFR ;)
|
👍 |
| public function dump(ParsedExpression $expression) | ||
| { | ||
| return $expression->getNodes()->dump(); | ||
| } |
There was a problem hiding this comment.
I would remove this method and add a dump() method on ParsedExpression instead.
There was a problem hiding this comment.
@fabpot I addressed your comments.
|
|
||
| public function dump() | ||
| { | ||
| return sprintf('<Dumping a "%s" instance is not supported yet>', get_class($this)); |
There was a problem hiding this comment.
Wouldn't it be better to just throw an exception here? As the dumped expression is not usable anyway.
There was a problem hiding this comment.
I don't know. It was easier to be able to see a "preview" while developing than an exception.
But indeed, as the expression is not valid, let throw an expression. (done)
| @@ -75,4 +75,27 @@ public function evaluate($functions, $values) | ||
|
|
||
| return $results; | ||
| } | ||
|
|
||
| public function dump() |
There was a problem hiding this comment.
Other public method don't have PHPDoc... So it tried to be consistent here ;)
|
Thank you @lyrixx. |
This PR was merged into the 3.2-dev branch. Discussion ---------- [ExpressionLanguage] Added a way to dump the AST | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Because it's convenient and it makes easier to inline results of sub/smaller part of the main expression. Commits ------- 87af6e5 [ExpressionLanguage] Added a way to dump AST
…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
Because it's convenient and it makes easier to inline results of sub/smaller part of the main expression.