Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[VarDumper] Add support of named arguments todd() anddump() to display a label#48432

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

Merged

Conversation

alexandre-daubois
Copy link
Member

@alexandre-dauboisalexandre-daubois commentedDec 1, 2022•
edited
Loading

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?no
TicketsN/A
LicenseMIT
Doc PRTodo

Following an idea from@nicolas-grekas, the goal here is to ease debugging withdd() anddump() 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 tofile:///home/alexandredaubois/PhpstormProjects/dummy_project/src/Command/TestCommand.php#L15.

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

javiereguiluz, 7ochem, Jeroeny, SVillette, deguif, artyuum, Jibbarth, bastnic, Koc, alexander-schranz, and 13 more reacted with thumbs up emojifancyweb, Kocal, Jibbarth, bastnic, evertharmeling, alexander-schranz, steef, samuel-gerard, SiM07, bytehead, and 8 more reacted with heart emojijaviereguiluz, 7ochem, lyrixx, alexander-schranz, steef, ging-dev, and Jir4 reacted with rocket emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

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 ;)

GromNaN and alexandre-daubois reacted with laugh emoji
@@ -37,13 +37,13 @@ class VarDumper
*/
private static $handler;

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

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
publicstaticfunction dump(mixed$var,string$sectionName =null)
/**
* @param string|null$name
*/
publicstaticfunction dump(mixed$var,/*string $name = null*/)

Copy link
MemberAuthor

@alexandre-dauboisalexandre-dauboisDec 2, 2022•
edited
Loading

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

@nicolas-grekasnicolas-grekasDec 2, 2022•
edited
Loading

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, seehttps://3v4l.org/2cUek

Copy link
Member

@GromNaNGromNaN 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.

alexandre-daubois reacted with thumbs up emoji
@@ -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
publicstaticfunction dump(mixed$var,string$sectionName =null)
/**
* @param string|null$name
*/
publicstaticfunction 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-dauboisalexandre-daubois changed the title[VarDumper] Add support of named arguments todd() anddump() to display a "section name"[VarDumper] Add support of named arguments todd() anddump() to display a labelDec 2, 2022
@lyrixx
Copy link
Member

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
MemberAuthor

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 tweakdump() anddd() behaviours sounds great!

lyrixx reacted with thumbs up emoji

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 ($varsas$key =>$v) {
VarDumper::dump($v,is_numeric($key) ?null :$key);
}
$i =0;
foreach ($varsas$key =>$v) {
VarDumper::dump($v,is_numeric($key) ?"$i" :"$key@$i");
$i++;
}

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
MemberAuthor

@alexandre-dauboisalexandre-dauboisDec 6, 2022•
edited
Loading

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 ($varsas$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

@GromNaNGromNaNDec 6, 2022•
edited
Loading

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
MemberAuthor

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

Nice feature !

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

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 keepinga 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.

alexandre-daubois reacted with thumbs up emojialexandre-daubois reacted with rocket emoji
Comment on lines 50 to 61
foreach ($vars as $key => $v) {
VarDumper::dump($v, is_numeric($key) ? null : $key);
}

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)

@GromNaN
Copy link
Member

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

nicolas-grekas and alexandre-daubois reacted with thumbs up emoji

@alexandre-dauboisalexandre-dauboisforce-pushed thefeat/dd-section-name branch 5 times, most recently from7dbba29 tob1e4214CompareDecember 11, 2022 11:13
Copy link
Member

@nicolas-grekasnicolas-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!

@alexandre-daubois
Copy link
MemberAuthor

alexandre-daubois commentedDec 15, 2022•
edited
Loading

Everything has been updated according to your feedback. Thanks!

@GromNaN
Copy link
Member

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

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@drupoldrupoldrupol left review comments

@GromNaNGromNaNGromNaN left review comments

@stofstofstof left review comments

@azjezzazjezzazjezz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

8 participants
@alexandre-daubois@lyrixx@drupol@GromNaN@nicolas-grekas@stof@azjezz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp