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 dd() helper == dump() + exit() #26970

Merged
merged 1 commit into from Apr 19, 2018

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Apr 18, 2018

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

By popular demand, I feel like we should reconsider our refusal for a dd() global helper.
For past references, see #26965, #26906, #13657, #17267, #19096.

@javiereguiluz

We gave some reasons to not add this function in the past ... but when you receive so many requests from the community to add it, it's time to rethink your decisions. Nicolas, thanks for reconsidering this and for adding the function.

@onEXHovia

This comment has been minimized.

onEXHovia commented Apr 18, 2018

I think there will be a similar number of pros and cons.
Should we add TwigFunction by analogy with dump 🤔

@pamil

This comment has been minimized.

Contributor

pamil commented Apr 18, 2018

Seeing dd() anywhere in the code for the first time would cause nothing else but confusion. If one wants to save keystrokes, why not create a macro in IDE (or use a debugger)?

@iltar

This comment has been minimized.

Contributor

iltar commented Apr 18, 2018

  • It doesn't add complexity
  • It's optional
  • It's faster than hooking up the debugger
  • Not everyone has experience with a debugger
  • It doesn't replace a debugger and you're not forced to use this

I see no downsides and maintenance isn't an issue 👍

@kunicmarko20

This comment has been minimized.

kunicmarko20 commented Apr 18, 2018

@iltar But you have dump(), as @pamil said you can create IDE macro. If this gets accepted won't it start an avalanche of people wanting shortcuts for something?

@javiereguiluz javiereguiluz changed the title from [VarDumper] Add dd() helper == dump() + die() to [VarDumper] Add dd() helper == dump() + exit() Apr 18, 2018

@lsmith77 lsmith77 self-requested a review Apr 18, 2018

@lsmith77

I think the potential for harm is very small and the feature has been requested a lot.

@lyrixx

lyrixx approved these changes Apr 18, 2018

👍

@Pierstoval

This comment has been minimized.

Contributor

Pierstoval commented Apr 18, 2018

Creating a macro in the IDE is part of the doc, not the framework, and not asked as much as dd(). I'm totally approving this feature as it's almost like "everyone's asking for it", where debugger usage, even though better, can be cumbersome for lots of devs.

@stof

SourceContextProvider must be updated to whitelist dd as well when looking for the source of the dump. Otherwise, the dump will be reported as coming from inside dd which is useless.

@iltar

This comment has been minimized.

Contributor

iltar commented Apr 18, 2018

@kunicmarko20 Not everyone is always developing with an IDE, don't forget the notepad(++), Vim, atom etc developers. Also important to note down, is that the var-dumper can be used as stand-alone. Meaning if devs like the symfony dump(), they can use this package. This reaches far beyond experienced developers with debuggers, as you'll probably be reaching hobby devs, wordpress/joomla/drupal devs etc.

When you're comparing the full feature set of the var-dumper component vs other dumpers, then a dump and die functionality is quite common and desired. It takes a few lines to add the function, but adds a big feeling of completion for the developers that would otherwise have used a dd()-similar function from another framework.

I think it's quite nice to have a "full" solution coming from Symfony, making it even better 👍

@pamil

This comment has been minimized.

Contributor

pamil commented Apr 18, 2018

After reading the reasoning behind introducing this function I think it's a good idea in fact and with almost no potential for harm. 👍

@deflock

This comment has been minimized.

deflock commented Apr 18, 2018

The feature itself is must have, but not sure about name, confuses a bit.

@iltar Can you explain "It's optional"? What if I want dump() but not dd() in global scope?

@Meroje

This comment has been minimized.

Meroje commented Apr 18, 2018

@nicolas-grekas @DojoGeekRA chrome 65 removed the need for an error code to render the preview that was introduced with 62 https://bugs.chromium.org/p/chromium/issues/detail?id=785050

@haydenk

This comment has been minimized.

haydenk commented Apr 18, 2018

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

@lyrixx

This comment has been minimized.

Member

lyrixx commented Apr 18, 2018

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

because dump is variadic

@haydenk

This comment has been minimized.

haydenk commented Apr 18, 2018

@lyrixx Ah, good point.

@joubertredrat

This comment has been minimized.

joubertredrat commented Apr 18, 2018

Oh, a enem..., erm, a friend @taylorotwell here? Hey, let's drink coffee?

About function, I think that is best to be more verbosity, how dump_die, dump_exit or dump_output as example, what you think guys?

Why not add an optional argument to dump() to exit when it's done instead of creating a whole new function?

Because dump today support multiple arguments.

@kunicmarko20

This comment has been minimized.

kunicmarko20 commented Apr 18, 2018

If people think we need a shortcut then it should be small, dd is the best choice for this. If it is longer then I can write dump();die(); myself.

@Pierstoval

This comment has been minimized.

Contributor

Pierstoval commented Apr 18, 2018

What a mess of a conversation just for a DX initiative that has been demanded for a long time, and still lots of arguments against.

I think this is part of the small changes that can really conciliate lots of dev communities and improve "classic" debug experience as well for people not using complex xdebug setups or full-stack IDEs. I mean, when one debugs using VIM, a simple dd() is really straightforward.

@sroze

sroze approved these changes Apr 18, 2018

@akalongman

This comment has been minimized.

akalongman commented Apr 18, 2018

+1 for dump_die()

@Majkl578

This comment has been minimized.

Contributor

Majkl578 commented Apr 18, 2018

Maybe rename Symfony\Component\VarDumper\VarDumper::dump() to D::d() instead?

@akovalyov akovalyov referenced this pull request Apr 18, 2018

Merged

Remove dd if present #2

@carusogabriel

🍿

@antoniocambados

This comment has been minimized.

antoniocambados commented Apr 18, 2018

I don't miss a dd() shortcut but I can barely think of any serious objection. The proposed name is convenient and matches other frameworks, so I'd stick with it.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 19, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit a55916a into symfony:master Apr 19, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 19, 2018

feature #26970 [VarDumper] Add dd() helper == dump() + exit() (nicola…
…s-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[VarDumper] Add dd() helper == dump() + exit()

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

By popular demand, I feel like we should reconsider our refusal for a `dd()` global helper.
For past references, see #26965, #26906, #13657, #17267, #19096.

Commits
-------

a55916a [VarDumper] Add dd() helper == dump() + die()
@Wirone

This comment has been minimized.

Wirone commented Apr 19, 2018

@Pierstoval

I mean, when one debugs using VIM, a simple dd() is really straightforward.

Unless they're in command mode 😂

@sstok

This comment has been minimized.

Contributor

sstok commented Apr 19, 2018

Lets be clear, a function/method/class should always have a name that describes it's purpose, it helps the developers and prevents mistakes.

dd, a or Foobar are not descriptive (unless you have a Bar named foo :trollface: - insert Jonh Taffer meme here ) but it's important to understand why we use these undescriptive names, because it doesn't matter. Foobar is only used as an example for testing, dd() is something you only use during development!! It prints the result and stops execution, please don't tell me your production does this kind of logic 😐 (you properly use a proper solution for a specific problem and let the framework handle exceptions and the response handling).

Clean Code is about production code and tests, not about simple hacks to find out why something isn't working. Not everyone has the power of a debugger, sometimes the debugger is not able to reach the code (actually happened to me a few time) or doesn't show all details (or the details are to verbose), by using the VarDumper you can easily solve this problem, and having a simple method to dump and prevent further execution is perfectly fine for development and troubleshooting.

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:dd branch Apr 19, 2018

@gragio

This comment has been minimized.

gragio commented Apr 19, 2018

Good job! Thanks @nicolas-grekas

@jpauli

This comment has been minimized.

Contributor

jpauli commented Apr 19, 2018

👍

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 19, 2018

I'm locking the thread because the issue has been resolved.
Thank you all for participating!

@symfony symfony locked as resolved and limited conversation to collaborators Apr 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.