Skip to content

[VarDumper] Dumping exceptions is now more compact #19289

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 28, 2016

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 5, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

Before:
before

@javiereguiluz
Copy link
Member

👍

This looks much better. Thanks Nicolas!


I'd also suggest to not use 1-letter variable names because it makes code hard to understand and maintain in the long term.

@nicolas-grekas nicolas-grekas force-pushed the dump-compact branch 2 times, most recently from 6776dd0 to 01dfc6c Compare July 5, 2016 12:34
@nicolas-grekas
Copy link
Member Author

New "after":
after


for ($j += $trace->numberingOffset - $i++; isset($frames[$i]); ++$i, --$j) {
$call = isset($frames[$i]['function']) ? (isset($frames[$i]['class']) ? $frames[$i]['class'].$frames[$i]['type'] : '').$frames[$i]['function'].'()' : '???';

$a[Caster::PREFIX_VIRTUAL.$j.'. '.$call.$lastCall] = new FrameStub(
$k = $call.$lastCall;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename these variables to more meaningful names

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@nicolas-grekas nicolas-grekas force-pushed the dump-compact branch 5 times, most recently from 514ad94 to 90fd3b0 Compare July 5, 2016 13:45
foreach ($frame->value as $label => $frame) {
}
} else {
$label = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this statement be moved before the if condition to prevent the else statement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, no need for any if/else in fact

@nicolas-grekas
Copy link
Member Author

ping @symfony/deciders

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 18, 2016

Status: needs work
I'd like to try a few more ideas when dumping arguments.

@nicolas-grekas
Copy link
Member Author

Updated to be compact also when args are dumped. Test cases added.

Status: needs review

@nicolas-grekas nicolas-grekas merged commit 19e9cbe into symfony:master Jul 28, 2016
nicolas-grekas added a commit that referenced this pull request Jul 28, 2016
…colas-grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[VarDumper] Dumping exceptions is now more compact

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Before:
![before](https://cloud.githubusercontent.com/assets/243674/16578686/2c7285b0-429c-11e6-9139-293e0c899d43.png)

Commits
-------

19e9cbe [VarDumper] Dumping exceptions is now more compact
@nicolas-grekas nicolas-grekas deleted the dump-compact branch July 28, 2016 07:26
@fabpot fabpot mentioned this pull request Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants