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] Added a way to dump the AST#19013
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
| $str .=sprintf('%s,',$pair['value']->dump()); | ||
| } | ||
| $str =rtrim($str,','); |
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.
you can return here directly (same everywhere)
stof commentedJun 9, 2016
Cannot be accepted without tests |
lyrixx commentedJun 10, 2016
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
when int keys are used but not in numerical order, this won"t work, isn't it?
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 don't see why it would not work. but if it's the case, how could I fix it?
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.
2dbc349 to23a5c66Compare| protectedfunctiondumpEscape($value) | ||
| { | ||
| returnstr_replace(['\\','"'], ['\\\\','\"'],$value); |
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.
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.
I fixed another one too, and added more tests. RFR ;)
nicolas-grekas commentedJun 12, 2016
👍 |
| publicfunctiondump(ParsedExpression$expression) | ||
| { | ||
| return$expression->getNodes()->dump(); | ||
| } |
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 would remove this method and add adump() method onParsedExpression instead.
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.
@fabpot I addressed your comments.
| publicfunctiondump() | ||
| { | ||
| returnsprintf('<Dumping a "%s" instance is not supported yet>',get_class($this)); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
| return$results; | ||
| } | ||
| publicfunctiondump() |
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.
You should add a phpdoc here.
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.
Other public method don't have PHPDoc... So it tried to be consistent here ;)
fabpot commentedJun 13, 2016
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
Uh oh!
There was an error while loading.Please reload this page.
Because it's convenient and it makes easier to inline results of sub/smaller part of the main expression.