Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] fix trailling comma when dumping an exception#24735
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
37554f3 to5ae1073Compare
ogizanagi left a comment• 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.
More broadly: it only uses the trailing comma for actual arrays, which is what targeted the initial feature anyway. Not adding a trailing comma to each line.
👍
nicolas-grekas commentedOct 29, 2017
Not sure: as@lyrixx mentioned, the comma is needed when dumping on a single line. Then, it's also useful for objects, isn't it? |
ogizanagi commentedOct 29, 2017 • 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.
Perhaps, butAFAIU, nested structures aren't dumped inlined, and that's probably not something we should target as it'll be pretty unreadable and useless to me. Seeformat.php code#!/usr/bin/env php<?phpuseSymfony\Bridge\Monolog\Formatter\ConsoleFormatter;useSymfony\Component\Console\Input\ArgvInput;useSymfony\Component\Console\Input\InputDefinition;useSymfony\Component\Console\Input\InputOption;useSymfony\Component\Console\Output\ConsoleOutput;useSymfony\Component\OptionsResolver\OptionsResolver;require__DIR__.'/vendor/autoload.php';$input =newArgvInput(null, (newInputDefinition([newInputOption('multiline')])));$formatter =newConsoleFormatter(['multiline' =>$input->getOption('multiline')]);$std =newstdClass();$std->foo =123;$std->bar ='bar';(newConsoleOutput())->writeln($formatter->format(['level' =>100,'level_name' =>'info','message' =>'foo','datetime' =>new \DateTime(),'channel' =>'app','context' => ['date' =>new \DateTime(),'opt' =>newOptionsResolver(),'std' =>$std, ],'extra' => [],]));
10:46:31 info [app] foo ["date" => DateTime @1509270391 {date:2017-10-2910:46:31.987557 Europe/Zurich (+01:00)},"opt" =>Symfony\Component\OptionsResolver\OptionsResolver { …},"std" => { …}] [] whereas 10:46:38 info [app] foo["date" => DateTime @1509270398 { date:2017-10-2910:46:38.564727 Europe/Zurich (+01:00) },"opt" =>Symfony\Component\OptionsResolver\OptionsResolver { -defined: [] -defaults: [] -required: [] -resolved: [] -normalizers: [] -allowedValues: [] -allowedTypes: [] -lazy: [] -calling: [] -locked: false },"std" => { +"foo":123 +"bar":"bar" }][] So IMHO it's okay. |
| protectedfunctionendValue(Cursor$cursor) | ||
| { | ||
| if (self::DUMP_TRAILING_COMMA &$this->flags &&0 <$cursor->depth) { | ||
| $isStubArray = (Stub::ARRAY_INDEXED ===$cursor->hashType || Stub::ARRAY_ASSOC ===$cursor->hashType); |
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.
should be an "if" that wraps the other "if"s
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.
WDYM?
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.
diff --git a/src/Symfony/Component/VarDumper/Dumper/CliDumper.php b/src/Symfony/Component/VarDumper/Dumper/CliDumper.phpindex 48f9dc1108..5bf89112e6 100644--- a/src/Symfony/Component/VarDumper/Dumper/CliDumper.php+++ b/src/Symfony/Component/VarDumper/Dumper/CliDumper.php@@ -526,11 +526,12 @@ class CliDumper extends AbstractDumper protected function endValue(Cursor $cursor) {- $isStubArray = (Stub::ARRAY_INDEXED === $cursor->hashType || Stub::ARRAY_ASSOC === $cursor->hashType);- if (self::DUMP_TRAILING_COMMA & $this->flags && 0 < $cursor->depth && $isStubArray) {- $this->line .= ',';- } elseif (self::DUMP_COMMA_SEPARATOR & $this->flags && 1 < $cursor->hashLength - $cursor->hashIndex && $isStubArray) {- $this->line .= ',';+ if (Stub::ARRAY_INDEXED === $cursor->hashType || Stub::ARRAY_ASSOC === $cursor->hashType) {+ if (self::DUMP_TRAILING_COMMA & $this->flags && 0 < $cursor->depth) {+ $this->line .= ',';+ } elseif (self::DUMP_COMMA_SEPARATOR & $this->flags && 1 < $cursor->hashLength - $cursor->hashIndex) {+ $this->line .= ',';+ } } $this->dumpLine($cursor->depth, true);
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.
Thanks@ogizanagi, done@nicolas-grekas.
5ae1073 to0456475Compare0456475 tofc3fe7fComparenicolas-grekas commentedNov 1, 2017
Thank you@Simperfit. |
…(Simperfit)This PR was merged into the 3.3 branch.Discussion----------[VarDumper] fix trailling comma when dumping an exception| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yesish| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24581| License | MIT| Doc PR | OThis PR is fixing a behaviour when a exception got every single line with a comma.Commits-------fc3fe7f [VarDumper] fix trailling comma when dumping an exception
This PR is fixing a behaviour when a exception got every single line with a comma.