[2.3] [Console] TableHelper #6368

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
@umpirsky
Contributor

umpirsky commented Dec 15, 2012

When building a console application it may be useful to display tabular data.

TableHelper can display table header and rows, customizable alignment of columns, cell padding and colors.

Basic usage example:

$table = $app->getHelperSet()->get('table');
$table
    ->setHeaders(array('ISBN', 'Title', 'Author'))
    ->setRows(array(
        array('99921-58-10-7', 'Divine Comedy', 'Dante Alighieri'),
        array('9971-5-0210-0', 'A Tale of Two Cities', 'Charles Dickens'),
        array('960-425-059-0', 'The Lord of the Rings', 'J. R. R. Tolkien'),
        array('80-902734-1-6', 'And Then There Were None', 'Agatha Christie'),
    ))
;
$table->render($output);

Output:
table

If this PR gets merged I will submit doc PR as well.

I'm sure there is a plenty of room for improvements so any feedback is welcome.

@stloyd

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ }
+
+ $this->renderColumnSeparator();
+ for ($column = 0; $column < $this->getNumberOfColumns(); $column++) {

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 15, 2012

Contributor
for ($column = 0, $count = $this->getNumberOfColumns(); $column < $count; $column++) {
@stloyd

stloyd Dec 15, 2012

Contributor
for ($column = 0, $count = $this->getNumberOfColumns(); $column < $count; $column++) {

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 15, 2012

Contributor

Fixed, thanks!

@umpirsky

umpirsky Dec 15, 2012

Contributor

Fixed, thanks!

@stloyd

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ }
+
+ $markup = $this->edgeChar;
+ for ($column = 0; $column < $this->getNumberOfColumns(); $column++) {

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 15, 2012

Contributor
for ($column = 0, $count = $this->getNumberOfColumns(); $column < $count; $column++) {
@stloyd

stloyd Dec 15, 2012

Contributor
for ($column = 0, $count = $this->getNumberOfColumns(); $column < $count; $column++) {
@stloyd

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ *
+ * @param string $verticalBorderChar
+ */
+ public function setVerticalBorderChar($verticalBorderChar)

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 15, 2012

Contributor

Wrong indentation.

@stloyd

stloyd Dec 15, 2012

Contributor

Wrong indentation.

@stloyd

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ */
+ public function setBackgroundChar($backgroundChar)
+ {
+ $this->backgroundChar = $backgroundChar;

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Dec 15, 2012

Contributor

IMO you should add return $this; too all set*() methods.

@stloyd

stloyd Dec 15, 2012

Contributor

IMO you should add return $this; too all set*() methods.

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 15, 2012

Contributor

I agree, forgot them. Fixed now, thanks.

@umpirsky

umpirsky Dec 15, 2012

Contributor

I agree, forgot them. Fixed now, thanks.

@michelsalib

This comment has been minimized.

Show comment Hide comment
@michelsalib

michelsalib Dec 16, 2012

Contributor

Looks very useful 👍

Contributor

michelsalib commented Dec 16, 2012

Looks very useful 👍

@lyrixx

View changes

src/Symfony/Component/Console/Tests/Helper/TableHelperTest.php
+ $this->assertEquals($expected, $this->getOutputContent($output));
+ }
+
+ public static function testRenderProvider()

This comment has been minimized.

Show comment Hide comment
@lyrixx

lyrixx Dec 16, 2012

Member

No need for static here.

@lyrixx

lyrixx Dec 16, 2012

Member

No need for static here.

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 16, 2012

Contributor

Fixed, thanks.

@umpirsky

umpirsky Dec 16, 2012

Contributor

Fixed, thanks.

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Dec 16, 2012

Member

I don't like this implementation: your helper is stateful, meaning that using it twice in a command can led to weird side-effects as it will still contain some stuff from the previous table you rendered (for instance reusign the custom spearator for the second table)

Member

stof commented Dec 16, 2012

I don't like this implementation: your helper is stateful, meaning that using it twice in a command can led to weird side-effects as it will still contain some stuff from the previous table you rendered (for instance reusign the custom spearator for the second table)

@umpirsky

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 16, 2012

Contributor

@stof Yes, I am aware of that, and I think users should be as well. Other helpers like `ProgressHelper' are no exception. The idea behind this helpers is to be stateful.

For example, if you want to render tables with green borders, you set green border format and they will be green until you change border format. I don't see this as a weird side-effect.

Other then that, I don't see a use case when this is not expected behavior, table style is usually standardized per application. And resetting all values to default is as easy as creating a new TableHelper instance.

Contributor

umpirsky commented Dec 16, 2012

@stof Yes, I am aware of that, and I think users should be as well. Other helpers like `ProgressHelper' are no exception. The idea behind this helpers is to be stateful.

For example, if you want to render tables with green borders, you set green border format and they will be green until you change border format. I don't see this as a weird side-effect.

Other then that, I don't see a use case when this is not expected behavior, table style is usually standardized per application. And resetting all values to default is as easy as creating a new TableHelper instance.

+ /**
+ * {@inheritDoc}
+ */
+ public function getName()

This comment has been minimized.

Show comment Hide comment
@piotrpasich

piotrpasich Dec 21, 2012

I think this method should be placed higher in the file.

@piotrpasich

piotrpasich Dec 21, 2012

I think this method should be placed higher in the file.

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Jan 7, 2013

Contributor

@umpirsky public methods must be declared before protected & private according to Symfony CS.

@stloyd

stloyd Jan 7, 2013

Contributor

@umpirsky public methods must be declared before protected & private according to Symfony CS.

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Jan 7, 2013

Contributor

@piotrpasich @stloyd I know, I just find this method not so important, so I put it at the bottom. Fixed now.

@umpirsky

umpirsky Jan 7, 2013

Contributor

@piotrpasich @stloyd I know, I just find this method not so important, so I put it at the bottom. Fixed now.

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

@umpirsky You were right, this method should be at the end like in the other helper classes.

@fabpot

fabpot Mar 23, 2013

Owner

@umpirsky You were right, this method should be at the end like in the other helper classes.

@piotrpasich

This comment has been minimized.

Show comment Hide comment
@piotrpasich

piotrpasich Dec 21, 2012

+1 from my side. Nice feature

+1 from my side. Nice feature

@henrikbjorn

This comment has been minimized.

Show comment Hide comment
@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

👍 could yield some nice debug commands :)

Contributor

henrikbjorn commented Dec 21, 2012

👍 could yield some nice debug commands :)

@Baachi

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ * @param array $row
+ * @param int $column
+ *
+ * @return int

This comment has been minimized.

Show comment Hide comment
@Baachi

Baachi Dec 21, 2012

Contributor

Should be integer :)

@Baachi

Baachi Dec 21, 2012

Contributor

Should be integer :)

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 21, 2012

Contributor

Why?
Short syntax is valid as well, check phpdoc.org:

@param bool|int $bar

and php.net:

var_dump((int) (25/7)); // int(3)
@umpirsky

umpirsky Dec 21, 2012

Contributor

Why?
Short syntax is valid as well, check phpdoc.org:

@param bool|int $bar

and php.net:

var_dump((int) (25/7)); // int(3)

This comment has been minimized.

Show comment Hide comment
@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

It dosent matter if it is valid or not, that is the way it is written in the rest of the phpdoc blocks :)

@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

It dosent matter if it is valid or not, that is the way it is written in the rest of the phpdoc blocks :)

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 21, 2012

Contributor

Fixed.

But, is this rule documented somewhere?
There are other places where short syntax is used https://gist.github.com/4352487.

Does this means that cast is done with:

(integer) $var;

as well?

Again, there are many short syntax casts in rest of the code.

@umpirsky

umpirsky Dec 21, 2012

Contributor

Fixed.

But, is this rule documented somewhere?
There are other places where short syntax is used https://gist.github.com/4352487.

Does this means that cast is done with:

(integer) $var;

as well?

Again, there are many short syntax casts in rest of the code.

This comment has been minimized.

Show comment Hide comment
@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

yes it is. There might be some slips but we can atleast try an be consistent where it is caught :)

@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

yes it is. There might be some slips but we can atleast try an be consistent where it is caught :)

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 21, 2012

Contributor

Can you provide link please? Don't tell me it's PSR :)

@umpirsky

umpirsky Dec 21, 2012

Contributor

Can you provide link please? Don't tell me it's PSR :)

This comment has been minimized.

Show comment Hide comment
@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

I am not saying it is a PSR or coding style. But i guess if you ack on integer instead of int, you find a lot more entries on it.

@henrikbjorn

henrikbjorn Dec 21, 2012

Contributor

I am not saying it is a PSR or coding style. But i guess if you ack on integer instead of int, you find a lot more entries on it.

@Baachi

This comment has been minimized.

Show comment Hide comment
@Baachi

Baachi Dec 21, 2012

Contributor

👍 Really nice!

Contributor

Baachi commented Dec 21, 2012

👍 Really nice!

@Burgov

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ $cellFormat,
+ str_pad(
+ $this->backgroundChar.$cell.$this->backgroundChar,
+ $this->getColumnWidth($column),

This comment has been minimized.

Show comment Hide comment
@Burgov

Burgov Dec 21, 2012

Contributor

this appears to be recalculated for every row that is rendered. I'm not sure about the performance, but I think this could be improved

@Burgov

Burgov Dec 21, 2012

Contributor

this appears to be recalculated for every row that is rendered. I'm not sure about the performance, but I think this could be improved

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 21, 2012

Contributor

Yes. I was thinking about caching getNumberOfColumns() and getColumnWidth() results. It will add some complexity, and improve performance. I will check some large table benchmarks and decide if this optimization have sense.
Thanks!

@umpirsky

umpirsky Dec 21, 2012

Contributor

Yes. I was thinking about caching getNumberOfColumns() and getColumnWidth() results. It will add some complexity, and improve performance. I will check some large table benchmarks and decide if this optimization have sense.
Thanks!

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Dec 22, 2012

Contributor

I optimized rendering a bit, here are the results of my little benchmark:

+----------------+-------------------+--------------------+
| columns x rows | Before (s)        | After (s)          |
+----------------+-------------------+--------------------+
| 3 x 10         | 0.009207010269165 | 0.0074009895324707 |
| 3 x 100        | 0.20063400268555  | 0.059360027313232  |
| 3 x 1000       | 14.763768911362   | 0.61125707626343   |
+----------------+-------------------+--------------------+
@umpirsky

umpirsky Dec 22, 2012

Contributor

I optimized rendering a bit, here are the results of my little benchmark:

+----------------+-------------------+--------------------+
| columns x rows | Before (s)        | After (s)          |
+----------------+-------------------+--------------------+
| 3 x 10         | 0.009207010269165 | 0.0074009895324707 |
| 3 x 100        | 0.20063400268555  | 0.059360027313232  |
| 3 x 1000       | 14.763768911362   | 0.61125707626343   |
+----------------+-------------------+--------------------+
@ohmybrew

This comment has been minimized.

Show comment Hide comment
@ohmybrew

ohmybrew Dec 28, 2012

+1 this is really useful to have.

+1 this is really useful to have.

@sukei

This comment has been minimized.

Show comment Hide comment
@sukei

sukei Dec 29, 2012

Contributor

+1 really nice feature!

Contributor

sukei commented Dec 29, 2012

+1 really nice feature!

@wouterj

This comment has been minimized.

Show comment Hide comment
@wouterj

wouterj Jan 1, 2013

Member

Isn't this table much to complicated for a great display in terminals? I think it is better to have something like:

ISBN           Title                     Author
=============  ========================  ================

99921-58-10-7  Divine Comedy             Dante Alighieri
9971-5-0210-0  A Tale of Two Cities      Charles Dickens
960-425-059-0  The Lord of the Rings     J. R. R. Tolkien
80-902734-1-6  And Then There Were None  Agatha Christie
Member

wouterj commented Jan 1, 2013

Isn't this table much to complicated for a great display in terminals? I think it is better to have something like:

ISBN           Title                     Author
=============  ========================  ================

99921-58-10-7  Divine Comedy             Dante Alighieri
9971-5-0210-0  A Tale of Two Cities      Charles Dickens
960-425-059-0  The Lord of the Rings     J. R. R. Tolkien
80-902734-1-6  And Then There Were None  Agatha Christie
@umpirsky

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Jan 1, 2013

Contributor

@wouterj Current implementation is what I find most convenient, and is a default now, you can easily change this with something like:

$table
    ->setEdgeChar(' ')
    ->setHorizontalBorderChar('=')
    ->setVerticalBorderChar(' ')
;

If community thinks some other default layout would be better, we can change it ofc.

Contributor

umpirsky commented Jan 1, 2013

@wouterj Current implementation is what I find most convenient, and is a default now, you can easily change this with something like:

$table
    ->setEdgeChar(' ')
    ->setHorizontalBorderChar('=')
    ->setVerticalBorderChar(' ')
;

If community thinks some other default layout would be better, we can change it ofc.

@stloyd

This comment has been minimized.

Show comment Hide comment
@stloyd

stloyd Jan 7, 2013

Contributor

@fabpot Skipping that small CS issue, I this it's good to be merged =)

Contributor

stloyd commented Jan 7, 2013

@fabpot Skipping that small CS issue, I this it's good to be merged =)

@fabpot

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ return $this;
+ }
+
+ public function setRow(array $row, $column)

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

This should be $column, array $row to be consistent with the rest of the framework.

@fabpot

fabpot Mar 23, 2013

Owner

This should be $column, array $row to be consistent with the rest of the framework.

@fabpot

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ /**
+ * Called after rendering to cleanup cache data.
+ */
+ private function afterRender()

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

what about calling it finishRendering?

@fabpot

fabpot Mar 23, 2013

Owner

what about calling it finishRendering?

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Maybe postRender or even cleanup?

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Maybe postRender or even cleanup?

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

cleanup sound good to me.

@fabpot

fabpot Mar 23, 2013

Owner

cleanup sound good to me.

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

Looks good to me. @umpirsky Can you take my comments into account, add a note in the CHANGELOG, and submit a PR for the documentation?

Owner

fabpot commented Mar 23, 2013

Looks good to me. @umpirsky Can you take my comments into account, add a note in the CHANGELOG, and submit a PR for the documentation?

@fabpot

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ }
+
+ if (isset($row[$column])) {
+ return mb_strlen($row[$column]);

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Owner

mbstring is not a required deps for Symfony, so you need to check and act accordingly like it's done elsewhere in the framework.

@fabpot

fabpot Mar 23, 2013

Owner

mbstring is not a required deps for Symfony, so you need to check and act accordingly like it's done elsewhere in the framework.

@lazyhammer

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ private $backgroundChar = ' ';
+ private $horizontalBorderChar = '-';
+ private $verticalBorderChar = '|';
+ private $edgeChar = '+';

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

How about cornerChar?

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

How about cornerChar?

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Or maybe crossingChar, since it's not just for corners.

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Or maybe crossingChar, since it's not just for corners.

@lazyhammer

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ {
+ $this->rows = array();
+
+ foreach ($rows as $row) {

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

return $this->addRows($rows); ?

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

return $this->addRows($rows); ?

@lazyhammer

View changes

src/Symfony/Component/Console/Helper/TableHelper.php
+ /**
+ * Sets background character, used for cell padding.
+ *
+ * @param string $backgroundChar

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

So maybe paddingChar according to it's purpose?

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

So maybe paddingChar according to it's purpose?

@umpirsky

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Mar 23, 2013

Contributor

@fabpot Thanks for useful comments, they are fixed.

Where in changelog this feature belongs, I see it's planned for 2.3?

I will try to submit documentation before the end of next week.

Contributor

umpirsky commented Mar 23, 2013

@fabpot Thanks for useful comments, they are fixed.

Where in changelog this feature belongs, I see it's planned for 2.3?

I will try to submit documentation before the end of next week.

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 25, 2013

Member

@umpirsky In the 2.3.0 section of the Console changelog.

Member

stof commented Mar 25, 2013

@umpirsky In the 2.3.0 section of the Console changelog.

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 26, 2013

Owner

@umpirsky Thanks for the update. I've reread the comments and I think @wouterj suggestion is good. So, can you change the default look to what he suggests? Also, what about defining some named configuration so that you can easily use pre-defined templates?

Owner

fabpot commented Mar 26, 2013

@umpirsky Thanks for the update. I've reread the comments and I think @wouterj suggestion is good. So, can you change the default look to what he suggests? Also, what about defining some named configuration so that you can easily use pre-defined templates?

@umpirsky

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Mar 26, 2013

Contributor

@fabpot Named configuration is great idea, I will try to fix something nice. Do you really like this borderless table? :)

For example, table layout I used is used by mysql client. It have sense for commands like doctrine:query:sql or doctrine:query:dql. On the other hand @wouterj layout can be handy for commands like container:debug or router:debug.

It would be nice to get more feedback from other people about default table layout.

Contributor

umpirsky commented Mar 26, 2013

@fabpot Named configuration is great idea, I will try to fix something nice. Do you really like this borderless table? :)

For example, table layout I used is used by mysql client. It have sense for commands like doctrine:query:sql or doctrine:query:dql. On the other hand @wouterj layout can be handy for commands like container:debug or router:debug.

It would be nice to get more feedback from other people about default table layout.

@pilot

This comment has been minimized.

Show comment Hide comment
@pilot

pilot Mar 26, 2013

Contributor

Mysql like table layouts is more convinient, but as @umpirsky say it's depend of commands for which try to use it, so maybe have a sense add a some params which configure table view from console?

Like a git log --pretty add a --style=mysql-table or --style=borderless

Contributor

pilot commented Mar 26, 2013

Mysql like table layouts is more convinient, but as @umpirsky say it's depend of commands for which try to use it, so maybe have a sense add a some params which configure table view from console?

Like a git log --pretty add a --style=mysql-table or --style=borderless

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 26, 2013

Owner

I think we won't have an agreement on which default table layout we should use; that's why named configuration is probably needed.

Owner

fabpot commented Mar 26, 2013

I think we won't have an agreement on which default table layout we should use; that's why named configuration is probably needed.

@umpirsky

This comment has been minimized.

Show comment Hide comment
@umpirsky

umpirsky Mar 26, 2013

Contributor

@fabpot But some configuration must be the default one.

Contributor

umpirsky commented Mar 26, 2013

@fabpot But some configuration must be the default one.

@bendavies

This comment has been minimized.

Show comment Hide comment
@bendavies

bendavies Mar 26, 2013

Contributor

if you are going to follow convention of most CLI tools, you start basic, then enhance from there, --pretty, --verbose etc...

Contributor

bendavies commented Mar 26, 2013

if you are going to follow convention of most CLI tools, you start basic, then enhance from there, --pretty, --verbose etc...

@umpirsky umpirsky referenced this pull request in symfony/symfony-docs Apr 6, 2013

Merged

Console table helper #2473

@fabpot fabpot closed this in 22adfdf Apr 7, 2013

@FredoVelcro

This comment has been minimized.

Show comment Hide comment
@FredoVelcro

FredoVelcro Oct 17, 2013

Lacks the ability to add a table footer or simply a dividing line to display totals for example...

Lacks the ability to add a table footer or simply a dividing line to display totals for example...

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Oct 17, 2013

Member

@FredoVelcro Please open a new ticket for requesting new features. Commenting on a PR merged 6 months ago will get lost

Member

stof commented Oct 17, 2013

@FredoVelcro Please open a new ticket for requesting new features. Commenting on a PR merged 6 months ago will get lost

@umpirsky umpirsky deleted the umpirsky:console-helper-table branch Oct 17, 2013

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