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

[FrameworkBundle] Long envvar value shortened to fit screen in AboutCommand #34768

Open
wants to merge 3 commits into
base: master
from

Conversation

@tuqqu
Copy link

tuqqu commented Dec 2, 2019

Q A
Branch? master
Bug fix? no
New feature? -
Deprecations? -
Tickets -
License MIT
Doc PR -

Fixed AboutCommand output by shortening environment variable value (replacing with ...) to fit the screen width if the value is too long.

Right now all output is a mess if it does not fit in the terminal window.

@tuqqu tuqqu force-pushed the tuqqu:about-command-fit-to-screen branch from f110fa8 to b006343 Dec 2, 2019
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 3, 2019

it's considerable to use the new Console dumper (#28898) for env var values, that would improve display as well.

The CliDumper has setMaxStringWidth, so if we can pass such a value (as calculated here) we can depend on existing building blocks :)

something like

diff --git a/Command/AboutCommand.php b/Command/AboutCommand.php
index b9fbe67..feee4e6 100644
--- a/Command/AboutCommand.php
+++ b/Command/AboutCommand.php
@@ -12,6 +12,7 @@
 namespace Symfony\Bundle\FrameworkBundle\Command;
 
 use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Helper\Dumper;
 use Symfony\Component\Console\Helper\Helper;
 use Symfony\Component\Console\Helper\TableSeparator;
 use Symfony\Component\Console\Input\InputInterface;
@@ -90,12 +91,13 @@ EOT
         ];
 
         if ($dotenv = self::getDotenvVars()) {
+            $dumper = new Dumper($io);
             $rows = array_merge($rows, [
                 new TableSeparator(),
                 ['<info>Environment (.env)</>'],
                 new TableSeparator(),
-            ], array_map(function ($value, $name) {
-                return [$name, $value];
+            ], array_map(function ($value, $name) use ($dumper) {
+                return [$name, $dumper($value, $maxLength)];
             }, $dotenv, array_keys($dotenv)));
         }
@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] Long envvar value shortened to fit screen in AboutC… [FrameworkBundle] Long envvar value shortened to fit screen in AboutCommand Dec 3, 2019
@tuqqu

This comment has been minimized.

Copy link
Author

tuqqu commented Dec 3, 2019

@ro0NL I'm not sure. Dumper::__invoke() does not allow to pass string's max length as the second parameter. Or are you suggesting to tweak its behaviour in this pr?

If so, dump's output is standing out with it's bright colours and double quotes from all the rest. Do we really want this?

If so, I could take a look into this.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 3, 2019

Or are you suggesting to tweak its behaviour in this pr?

yep :)

If so, dump's output is standing out with it's bright colours and double quotes from all the rest. Do we really want this?

IMHO yes, but perhaps post a screenshot to see what others think. Maybe it's too much indeed ... i was mostly triggered for being able to reuse CliDumper::setMaxStringWidth :)

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 3, 2019
@tuqqu

This comment has been minimized.

Copy link
Author

tuqqu commented Dec 3, 2019

@ro0NL I decided against using Dumper since we would rely on CliDumper::setMaxStringWidth() and it is part of VarDumper which is mostly a require-dev package, plus it adds a new dependency (on symfony/console).

So I rewrote it a bit and it looks quite nice. I added a pic
Screenshot 2019-12-03 at 23 33 12

Could you please review my code?

@ro0NL
ro0NL approved these changes Dec 3, 2019
Copy link
Contributor

ro0NL left a comment

decided against using Dumper since we would rely on CliDumper::setMaxStringWidth() and it is part of VarDumper which is mostly a require-dev package

good point, i forgot we need to account for the fallback implementation as well.

LGTM in general 👍

return max(18/* longest column text */, ...array_map('strlen', array_keys($dotenvVars)));
}
private static function adjustLength(string $value, int $maxLength): string

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 3, 2019

Contributor

truncate()

This comment has been minimized.

Copy link
@tuqqu

tuqqu Dec 4, 2019

Author

renamed

@@ -141,4 +144,28 @@ private static function getDotenvVars(): array
return $vars;
}
private static function getLongestColumnLength(array $dotenvVars): int

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 3, 2019

Contributor

imho it's fine to inline this above

This comment has been minimized.

Copy link
@tuqqu

tuqqu Dec 4, 2019

Author

fixed

@tuqqu tuqqu force-pushed the tuqqu:about-command-fit-to-screen branch from fc1c16e to 6bc65fc Dec 4, 2019
@tuqqu

This comment has been minimized.

Copy link
Author

tuqqu commented Dec 4, 2019

@ro0NL fixed all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.