Skip to content
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

[VarDumper] Add support of named arguments to dd() and dump() to display a label #48432

Merged
merged 1 commit into from Dec 15, 2022

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Dec 1, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR Todo

Following an idea from @nicolas-grekas, the goal here is to ease debugging with dd() and dump() by supporting named arguments passed to them. This will display a label, helping understand the goal/meaning of a dump.

Example in web browser:

image

Example in CLI:

image

The above example is clickable and points to file:///home/alexandredaubois/PhpstormProjects/dummy_project/src/Command/TestCommand.php#L15.

No more dd("First one", $var1, "Second var", $var2);!

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Send screenshots! We want to make the buzz around this ;)

src/Symfony/Component/VarDumper/Cloner/Data.php Outdated Show resolved Hide resolved
src/Symfony/Component/VarDumper/Cloner/Data.php Outdated Show resolved Hide resolved
src/Symfony/Component/VarDumper/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -37,13 +37,13 @@ class VarDumper
*/
private static $handler;

public static function dump(mixed $var)
public static function dump(mixed $var, string $sectionName = null)
Copy link
Member

Choose a reason for hiding this comment

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

$label
adding a new argument should be done in a BC way

Copy link
Member

Choose a reason for hiding this comment

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

This is a static method, so I don't think we have a BC issue here

Copy link
Member

Choose a reason for hiding this comment

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

I would call it $name (for argument name) or $key (for array key)
Like this.

Suggested change
public static function dump(mixed $var, string $sectionName = null)
/**
* @param string|null $name
*/
public static function dump(mixed $var, /*string $name = null*/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I would go for $label which is clearer and more suitable to the context of "displaying an information".

I'll do the change for the BC promise, as I didn't find any exception mentioning static methods in the prohibition to add an extra parameter to a public method. 👍

Copy link
Member

Choose a reason for hiding this comment

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

it's a BC break @stof, see https://3v4l.org/2cUek

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Very nice little feature.

I find the term "section" confusing. A section normally groups several things together. Here, there is only one variable for each name.

src/Symfony/Component/VarDumper/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -37,13 +37,13 @@ class VarDumper
*/
private static $handler;

public static function dump(mixed $var)
public static function dump(mixed $var, string $sectionName = null)
Copy link
Member

Choose a reason for hiding this comment

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

I would call it $name (for argument name) or $key (for array key)
Like this.

Suggested change
public static function dump(mixed $var, string $sectionName = null)
/**
* @param string|null $name
*/
public static function dump(mixed $var, /*string $name = null*/)

@@ -37,13 +37,13 @@ class VarDumper
*/
private static $handler;

public static function dump(mixed $var)
public static function dump(mixed $var, string $sectionName = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a static method, so I don't think we have a BC issue here

@alexandre-daubois alexandre-daubois changed the title [VarDumper] Add support of named arguments to dd() and dump() to display a "section name" [VarDumper] Add support of named arguments to dd() and dump() to display a label Dec 2, 2022
@lyrixx
Copy link
Member

lyrixx commented Dec 2, 2022

Oh, I like this so much. Can we leverage this, to be able to configuration the dumper? (may be in another PR). I would love to do something like

dd($value, max_depth: 4);

to automatically expand the 4 depth level

@alexandre-daubois
Copy link
Contributor Author

Big yes to me, @lyrixx! When talking about this idea, Nicolas seemed to have a few other ones that could definitely fit with this evolution in future PRs. A set of "reserved" named parameters to tweak dump() and dd() behaviours sounds great!

Comment on lines 50 to 61
foreach ($vars as $key => $v) {
VarDumper::dump($v, is_numeric($key) ? null : $key);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't be better to dump argument index as well here?

Suggested change
foreach ($vars as $key => $v) {
VarDumper::dump($v, is_numeric($key) ? null : $key);
}
$i = 0;
foreach ($vars as $key => $v) {
VarDumper::dump($v, is_numeric($key) ? "$i" : "$key@$i");
$i++;
}

Copy link
Member

Choose a reason for hiding this comment

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

When there is only one argument, I wouldn't dump the index as label. But when there are more, it might be nice to dump the index+1 even when it's numeric (+1 to start from 1, and the index should be the key, no need for $i)
(same for dump() of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was 👍 on this proposition, but:

$argc = count($vars);
foreach ($vars as $key => $v) {
    $displayLabel = 1 < $argc || !is_numeric($key);
    $label = is_numeric($key) ? $key+1 : $key;

    VarDumper::dump($v, $displayLabel ? $label : null);
}

This seems a bit complicated logic for the actual gain, what do you think?

Copy link
Member

@GromNaN GromNaN Dec 6, 2022

Choose a reason for hiding this comment

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

Like this?

if (isset($vars[0]) && 1 === count($vars)) {
    VarDumper::dump($vars[0]);
} else {
    // loop
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you!

image

@drupol
Copy link
Contributor

drupol commented Dec 2, 2022

Nice feature !

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Here are some comments.

I like the idea of using special names to configure the dump.

I'm keeping a branch from 2016 with all possible options.
Here is what I collected back then:

  • dump_destination
  • format (cli, html, txt, default)
  • max_items
  • max_depth
  • max_items_per_depth
  • max_string_length
  • max_string_width
  • collapse_depth
  • collapse_length
  • use_ref_handles
  • casters
  • replace_casters
  • filter
  • light_array
  • string_length
  • charset
  • trace
  • src_context
  • trace_args

We could even have an option builder:

$options = VarDumper::options()->trace()->maxDepth(4)->toArray();
dump($var, ...$options);

As a convention, it might be worth prefixing all options by an underscore?

Anyway, I'd just suggest considering this in a follow up PR to keep this one focused.

src/Symfony/Component/VarDumper/VarDumper.php Outdated Show resolved Hide resolved
Comment on lines 50 to 61
foreach ($vars as $key => $v) {
VarDumper::dump($v, is_numeric($key) ? null : $key);
}
Copy link
Member

Choose a reason for hiding this comment

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

When there is only one argument, I wouldn't dump the index as label. But when there are more, it might be nice to dump the index+1 even when it's numeric (+1 to start from 1, and the index should be the key, no need for $i)
(same for dump() of course)

src/Symfony/Component/VarDumper/Cloner/Data.php Outdated Show resolved Hide resolved
@GromNaN
Copy link
Member

GromNaN commented Dec 4, 2022

I like the idea of using special names to configure the dump.

Anyway, I'd just suggest considering this in a follow up PR to keep this one focused.

Let's discuss this other feature in #48474

@alexandre-daubois alexandre-daubois force-pushed the feat/dd-section-name branch 5 times, most recently from 7dbba29 to b1e4214 Compare December 11, 2022 11:13
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I only have minor CS suggestions. Thanks!

src/Symfony/Component/VarDumper/VarDumper.php Outdated Show resolved Hide resolved
@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Dec 15, 2022

Everything has been updated according to your feedback. Thanks!

@GromNaN
Copy link
Member

GromNaN commented Dec 15, 2022

Names starting with _ should be forbidden and reserved to the futur options.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Names starting with _ should be forbidden and reserved to the futur options.

let's do this in the PR that will follow up the issue you opened (same for the "no-args" behavior that needs to be adjusted if we follow your idea of a builder)

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

}

if (1 < func_num_args()) {
return func_get_args();
if (isset($vars[0]) && 1 === count($vars)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

count($vars) is called twice, maybe use an intermediary var to avoid doing the job twice?

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.

None yet

8 participants