[Console] Make pretty the `box` style table #25456

maidmaid · 2017-12-12T09:25:19Z

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

The console component is the 2nd most popular Symfony component with 70M of downloads. I think such composant has to provide faultless tables, with perfect management of borders, especially since the new box style table has been merged (cf #25301).

javiereguiluz

Before making more changes, let's wait and see if others agree with this feature. But if they agree, we should decided if we make all separators configurable (i.e. horizontal and vertical borders).

In other libraries (e.g. https://github.com/Automattic/cli-table) they define these chars:

      chars: {
          'top': '─'
        , 'top-mid': '┬'
        , 'top-left': '┌'
        , 'top-right': '┐'
        , 'bottom': '─'
        , 'bottom-mid': '┴'
        , 'bottom-left': '└'
        , 'bottom-right': '┘'
        , 'left': '│'
        , 'left-mid': '├'
        , 'mid': '─'
        , 'mid-mid': '┼'
        , 'right': '│'
        , 'right-mid': '┤'
        , 'middle': '│'
      }

javiereguiluz · 2017-12-12T09:31:20Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     *
+     * @return $this
+     */
+    public function setCrossingChars(string $cross, string $topLeft = null, string $topMiddle = null, string $topRight = null, string $middleRight = null, string $bottomRight = null, string $bottomMiddle = null, string $bottomLeft = null, string $middleLeft = null)


Instead of string $topLeft = null you can use ?string $topLeft because in master we use PHP 7.1, right?

It's not the same meaning in fact. I want to make optional these params.

function f(?string $p) {} f(); // Uncaught Error: Too few arguments to function f(), 0 passed in...

javiereguiluz · 2017-12-12T09:32:11Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     *
+     * @return string $crossingTopRightChar
+     */
+    public function getCrossingTopRightChar()


Return type-hints in these methods?

public function getCrossingTopRightChar(): string { ... }

Yes it would be better, but the others getters of this class don't have return type-hints.

not a big deal IMO, let's use new PHP features in new code
edit: I mean, let's add typehints for the newly added methods :)

maidmaid · 2017-12-12T10:16:35Z

@javiereguiluz Indeed, my current implementation provides only an unique style. The library which you mentionned allow to apply a different style between outside and inside borders. That doesn't look essential, but I like it. It can be useful for rendering a thinner separateur in the rows. I'm +1.

fabpot · 2017-12-12T17:39:37Z

The new style looks much better than the initial one. Looks great!

chalasr

Awesome 👍

stof · 2017-12-13T12:00:53Z

src/Symfony/Component/Console/Helper/Table.php

+            ? array($this->style->getCrossingMidLeftChar(), $this->style->getCrossingChar(), $this->style->getCrossingMidRightChar())
+            : (self::SEPARATOR_TOP === $type
+                ? array($this->style->getCrossingTopLeftChar(), $this->style->getCrossingTopMidChar(), $this->style->getCrossingTopRightChar())
+                : array($this->style->getCrossingBottomLeftChar(), $this->style->getCrossingBottomMidChar(), $this->style->getCrossingBottomRightChar()))


this nesting of ternary operators are hard to read IMO. I suggest using a if/elseif/else instead (and btw, this will also avoid the need to allocate arrays holding values just to unpack it immediately)

Is it better now?

stof · 2017-12-13T12:02:24Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     * </code>
+     *
+     * @param string      $cross       Crossing char (see #0 of example)
+     * @param string|null $topLeft     Top left char (see #1 of example), equal to $cross if undefined


if null. there is no undefined in PHP (E_TOO_MUCH_JAVASCRIPT 😄 ?)

chalasr · 2017-12-14T20:51:43Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     * Gets crossing top right char.
+     *
+     * @return string $crossingTopRightChar
+     */


You probably added it for consistency, but please remove this docblock as it does not give more info than the signature itself, same for next ones below.

Yes, it was for consistency with others methods. Docblocks removed.

Taluu · 2017-12-15T00:01:56Z

src/Symfony/Component/Console/Helper/TableStyle.php

@@ -132,6 +139,127 @@ public function getCrossingChar()
        return $this->crossingChar;


you should deprecate getCrossingChar() and setCrossingChar()

I deprecated setCrossingChar(). But not the getter which is always used as-is.

That's the point of a deprecation. Update the calls and trigger a deprecation message. :}

Taluu · 2017-12-15T00:06:20Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     *
+     * @return $this
+     */
+    public function setCrossingChars(string $cross, string $topLeft = null, string $topMid = null, string $topRight = null, string $midRight = null, string $bottomRight = null, string $bottomMid = null, string $bottomLeft = null, string $midLeft = null): self


imo I would rather use ?string as @javiereguiluz said (or even not nullable string) and not take crossingChar. This methid should be called with $this->setCrossingChars($cross, $cross, ...); (and thus slowly deprecate crossingChar)

Not sure to understand why use nullable string here... Could you to be more accurate please?

Because ?string $topLeft looks better than string $topLeft = null, it's more concise, and the ?string feature was introduced in modern PHP versions precisely to avoid doing string ... = null

Mmh I'm not sure that ?string $topLeft replaces string $topLeft = null. ?string simply allows string and null value, but it doesn't set a null default value.

function f(?string $p) {} f(); // Uncaught Error: Too few arguments to function f(), 0 passed in...

More info here: http://php.net/manual/en/migration71.new-features.php

Hum, didn't see the 0 in the schema, so my bad for the removal. But I still think that allowing a null value for the rest is not really good, and you should specify a value for all these other edges.

I mean, you CAN use the crossingChar for ALL the edges, but it shouldn't be implied if no value is given. Hence the "not allowing null values", so nothing too implicit.

allowing a null value for the rest is not really good

I think that the first param $crossingChar can be used as generic crossing char:

$style->setCrossingChars('+'); // vs $style->setCrossingChars('+', '+', '+', '+', '+', '+', '+', '+', '+');

i'd add this method above getCrossingChar

@ro0NL Moved.

ro0NL · 2017-12-16T15:49:06Z

src/Symfony/Component/Console/Helper/TableStyle.php

     */
    public function setCrossingChar($crossingChar)
    {
+        @trigger_error(sprintf('%s() is deprecated since version 4.1 and will be removed in 5.0. Use setCrossingChars() instead.', __METHOD__), E_USER_DEPRECATED);
+
        $this->crossingChar = $crossingChar;


should call $this->setCrossingChars($crossingChar); to update all properties

perhaps consider to not deprecate this method and make args in setCrossingChars required instead.

Oups, bad rebase. It's fixed.

Personally I prefer with optional args. Does this [Console] Make pretty the box style table #25456 (comment) convince you?

Mmh finally, I have changed my opinion :) I think that setCrossingChar() can be conserved and used as a shortcut of setCrossingChars(). Moreover, that simplifies a lot this PR. Could you check my second commit? If you disagree, I can remote it.

If you keeo the setCrossingChar, then maybe you should add setTopLeft, and so on. And then get rid of setCrossingChars.

I don't think so. If you don't use a generic crossing char by setting setCrossingChar(), you have to set all kinds of crossing char. And it's better if we can do it with only 1 call of setCrossingChars() instead 9 different calls (1 cross + 4 corners + 4 mids).

Isn't the idea of this PR to deprecate the "generic crossing char", in favor of using the specific ones ? The setCorssingChar should be imo just for the 0 (any "crossroad" delimiters). Using it as a generic should be deprecated.

It was. But the #25456 (comment) of @ro0NL looks relevant:

// we can set specific chars: setCrossingChars('┼', '┌', '┬', '┐', '┤', '┘', '┴', '└', '├'); // and we could also set generic char (useful for no-unicode): setCrossingChar('+'); // without this last one, we should write: setCrossingChars('+', '+', '+', '+', '+', '+', '+', '+', '+');

ro0NL · 2017-12-16T15:53:49Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     *
+     * @return $this
+     */
+    public function setCrossingChars(string $cross, string $topLeft = null, string $topMid = null, string $topRight = null, string $midRight = null, string $bottomRight = null, string $bottomMid = null, string $bottomLeft = null, string $midLeft = null): self


i'd add this method above getCrossingChar

ro0NL

┌─────────────┐
| lookin good |
└─────────────┘

ro0NL · 2017-12-16T16:38:31Z

src/Symfony/Component/Console/Helper/TableStyle.php

     */
    public function setCrossingChar($crossingChar)
    {
-        $this->crossingChar = $crossingChar;
+        @trigger_error(sprintf('%s() is deprecated since version 4.1 and will be removed in 5.0. Use setCrossingChars() instead.', __METHOD__), E_USER_DEPRECATED);


The method "%s()" is ... (note the quotes.. we want those :))

maidmaid · 2017-12-16T17:17:35Z

(@ro0NL For generating your message review, I hope that you have used:

(new Table(new ConsoleOutput()))->setStyle('box')->setRows([['lookin good']])->render();

😄)

maidmaid · 2018-01-05T03:16:59Z

@stof Do we need more changes with this PR?

maidmaid · 2018-03-11T04:56:36Z

Does this PR need some work still? (Travis failure not related)

ro0NL

minor comments :)

ro0NL · 2018-03-11T11:19:24Z

src/Symfony/Component/Console/Helper/TableStyle.php

     */
    public function setCrossingChar($crossingChar)
    {
-        $this->crossingChar = $crossingChar;
+        @trigger_error(sprintf('%s() is deprecated since Symfony 4.1 and will be removed in 5.0. Use setDefaultCrossingChar() instead.', __METHOD__), E_USER_DEPRECATED);


Method "%s()" is ...

ro0NL · 2018-03-11T11:21:44Z

src/Symfony/Component/Console/Helper/TableStyle.php

    /**
     * Sets crossing character.
     *
     * @param string $crossingChar
     *
     * @return $this
+     *
+     * @deprecated since Symfony 4.1, to be removed in 5.0. Use {@link setDefaultCrossingChar()} instead.


{@link method()} renders OK after compile? not sure it's needed / consistent:

https://github.com/symfony/symfony/search?l=PHP&q=%40deprecated+use+instead&type=&utf8=%E2%9C%93

Looks good :) and often used in the code base.

(annex comment: {@link method()} is not rendered on api.symfony.com. E.g. https://api.symfony.com/3.4/Symfony/Bundle/SecurityBundle/Security/FirewallContext.html#method_getContext)

I would remove the @link here as we've never done that before elsewhere AFAIK.

It's quite used in fact (e.g. on 3.4, more than 300 occurrences whose 13 in a @deprecated annotation) and it makes sense here IMO. Sure?

ok let's keep like this then :)

ostrolucky · 2018-03-12T23:48:32Z

src/Symfony/Component/Console/Helper/TableStyle.php

@@ -132,6 +194,46 @@ public function getCrossingChar()
        return $this->crossingChar;
    }

+    public function getCrossingTopRightChar(): string


Do we need these getters? I can't picture a use case for these.

I agree with @ostrolucky, I would remove all the getters.

But getters are used in Table class for rendering, they can't be removed.

I fear I'll might be the only one, but I'd rather use a single getter to get them all at once (from topLeft to midLeft).

+1 for @ogizanagi's idea. And let's mark it as internal.

fabpot

If this new style works on all OSes (including Windows), I would switch the default table theme to the new one.

fabpot · 2018-03-13T12:43:07Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     * Sets crossing characters.
+     *
+     * Example:
+     * <code>


We are not using HTML in desc, so this should be removed.

<code> tag is sometimes used yes (on master, 17 occurrences). This code-description can be useful IMO.

fabpot · 2018-03-13T12:43:36Z

src/Symfony/Component/Console/Helper/TableStyle.php

+     * @param string $bottomLeft  Bottom left char (see #7 of example)
+     * @param string $midLeft     Mid left char (see #8 of example)
+     *
+     * @return $this


Can be removed as the method signature already document this explicitly.

fabpot · 2018-03-13T12:44:19Z

src/Symfony/Component/Console/Helper/TableStyle.php

    /**
     * Sets crossing character.
     *
     * @param string $crossingChar
     *
     * @return $this
+     *
+     * @deprecated since Symfony 4.1, to be removed in 5.0. Use {@link setDefaultCrossingChar()} instead.


I would remove the @link here as we've never done that before elsewhere AFAIK.

fabpot · 2018-03-13T12:44:54Z

src/Symfony/Component/Console/Helper/TableStyle.php

@@ -132,6 +194,46 @@ public function getCrossingChar()
        return $this->crossingChar;
    }

+    public function getCrossingTopRightChar(): string


I agree with @ostrolucky, I would remove all the getters.

maidmaid · 2018-03-14T10:40:10Z

including Windows

It's supposed to work with DOS too (cf https://en.wikipedia.org/wiki/Box-drawing_character#DOS).

I would switch the default table theme to the new one

Yes! But switching to the new theme breaks some tests and it's more complicated than expected. I suggest to do that in an another PR.

Tobion · 2018-03-15T00:55:50Z

src/Symfony/Component/Console/Helper/TableStyle.php

     */
    public function setCrossingChar($crossingChar)
    {
-        $this->crossingChar = $crossingChar;
+        @trigger_error(sprintf('Method %s() is deprecated since Symfony 4.1 and will be removed in 5.0. Use setDefaultCrossingChar() instead.', __METHOD__), E_USER_DEPRECATED);


and will be removed in 5.0 should be removed

fabpot · 2018-03-17T15:09:07Z

Thank you @maidmaid.

This PR was merged into the 4.1-dev branch. Discussion ---------- [Console] Make pretty the `box` style table | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | / | License | MIT | Doc PR | / The console component is the 2nd most popular Symfony component with 70M of downloads. I think such composant has to provide faultless tables, with perfect management of borders, especially since the new `box` style table has been merged (cf #25301). ![screenshot from 2017-12-12 18-47-34](https://user-images.githubusercontent.com/4578773/33876488-6e1dc81c-df71-11e7-924f-d5e8078d957f.png) Commits ------- 41f52b3 Make pretty the `box` style table

robfrawley · 2018-03-18T06:03:15Z

@maidmaid This looks absolutely amazing! I'm excited to see more feature/improvement changes for the console component's output handling; this is an excellent addition. Thanks for the work to get it done!

@javiereguiluz

This PR was merged into the 4.1-dev branch. Discussion ---------- [Console] Add box-double table style | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#25456 (review) | License | MIT | Doc PR | / In symfony/symfony#25456 (review) @javiereguiluz suggested to have a complete configuration for style table. It was a nice idea, but not yet implemented. The box-drawing characters allow to combine different styles (like double/single lines). This PR give the possibility to use them. The previous symfony/symfony#25456 introduced the box-drawing characters; this PR exploits all their strength. ![image](https://user-images.githubusercontent.com/4578773/38136596-d8f4f46c-3462-11e8-9a0d-dcfae6039d18.png) See https://gist.github.com/maidmaid/3eb55afc4f2857cae89d7ac43d7943ae for some examples. Commits ------- 463f986c28 Add box-double table style

@javiereguiluz

This PR was merged into the 4.1-dev branch. Discussion ---------- [Console] Add box-double table style | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25456 (review) | License | MIT | Doc PR | / In #25456 (review) @javiereguiluz suggested to have a complete configuration for style table. It was a nice idea, but not yet implemented. The box-drawing characters allow to combine different styles (like double/single lines). This PR give the possibility to use them. The previous #25456 introduced the box-drawing characters; this PR exploits all their strength. ![image](https://user-images.githubusercontent.com/4578773/38136596-d8f4f46c-3462-11e8-9a0d-dcfae6039d18.png) See https://gist.github.com/maidmaid/3eb55afc4f2857cae89d7ac43d7943ae for some examples. Commits ------- 463f986 Add box-double table style

carsonbot added Status: Needs Review Console Feature labels Dec 12, 2017

javiereguiluz reviewed Dec 12, 2017

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Dec 12, 2017

maidmaid force-pushed the console-pretty-table branch 3 times, most recently from 6ebc50d to d7eec74 Compare December 13, 2017 05:24

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 13, 2017

chalasr approved these changes Dec 13, 2017

View reviewed changes

stof requested changes Dec 13, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Dec 13, 2017

maidmaid force-pushed the console-pretty-table branch from d7eec74 to a3f71c0 Compare December 14, 2017 04:37

chalasr reviewed Dec 14, 2017

View reviewed changes

Taluu suggested changes Dec 15, 2017

View reviewed changes

maidmaid force-pushed the console-pretty-table branch 3 times, most recently from 650c922 to a701379 Compare December 16, 2017 09:47

ro0NL reviewed Dec 16, 2017

View reviewed changes

maidmaid force-pushed the console-pretty-table branch from a701379 to 3e6d1e3 Compare December 16, 2017 16:31

carsonbot added Status: Reviewed and removed Status: Needs Work labels Dec 16, 2017

ro0NL approved these changes Dec 16, 2017

View reviewed changes

Simperfit approved these changes Dec 17, 2017

View reviewed changes

ro0NL mentioned this pull request Feb 19, 2018

[FrameworkBundle] Fix deprecation message format #26221

Closed

maidmaid force-pushed the console-pretty-table branch from 60f0c80 to f60296d Compare February 19, 2018 10:36

ro0NL approved these changes Mar 11, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 11, 2018

maidmaid force-pushed the console-pretty-table branch from f60296d to 528eacd Compare March 12, 2018 00:36

javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 12, 2018

ostrolucky reviewed Mar 12, 2018

View reviewed changes

fabpot requested changes Mar 13, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 13, 2018

maidmaid force-pushed the console-pretty-table branch from 528eacd to 59f8464 Compare March 14, 2018 10:44

Tobion reviewed Mar 15, 2018

View reviewed changes

Make pretty the box style table

41f52b3

maidmaid force-pushed the console-pretty-table branch from 59f8464 to 41f52b3 Compare March 17, 2018 08:57

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 17, 2018

fabpot approved these changes Mar 17, 2018

View reviewed changes

fabpot merged commit 41f52b3 into symfony:master Mar 17, 2018

maidmaid mentioned this pull request Mar 19, 2018

[WIP][Console] Set box style table as default #26596

Closed

javiereguiluz mentioned this pull request Mar 20, 2018

[Console] Make pretty the box style table symfony/symfony-docs#9456

Closed

maidmaid mentioned this pull request Mar 28, 2018

[Console] Add box-double table style #26693

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

		@@ -132,6 +139,127 @@ public function getCrossingChar()
		return $this->crossingChar;

[Console] Make pretty the box style table #25456

[Console] Make pretty the box style table #25456

Conversation

maidmaid commented Dec 12, 2017

javiereguiluz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

maidmaid Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

maidmaid commented Dec 12, 2017

fabpot commented Dec 12, 2017

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taluu Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid commented Dec 16, 2017

maidmaid commented Jan 5, 2018

maidmaid commented Mar 11, 2018

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

ogizanagi Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Console] Make pretty the `box` style table #25456

[Console] Make pretty the `box` style table #25456

chalasr Dec 12, 2017 •

edited

Loading

maidmaid Dec 13, 2017 •

edited

Loading

maidmaid Dec 16, 2017 •

edited

Loading

maidmaid Dec 16, 2017 •

edited

Loading

javiereguiluz Dec 16, 2017 •

edited

Loading

Taluu Dec 16, 2017 •

edited

Loading

maidmaid Dec 16, 2017 •

edited

Loading

maidmaid Dec 17, 2017 •

edited

Loading

ro0NL left a comment •

edited

Loading

maidmaid Mar 14, 2018 •

edited

Loading

ostrolucky Mar 14, 2018 •

edited

Loading

ogizanagi Mar 14, 2018 •

edited

Loading

maidmaid commented Mar 14, 2018 •

edited

Loading

robfrawley commented Mar 18, 2018 •

edited

Loading