Skip to content

[Console] [Helper] [Table] Columns styles #14044

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

Closed

Conversation

MAXakaWIZARD
Copy link
Contributor

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

This PR introduces ability to set styles for individual columns in table. For example, we can apply STR_PAD_LEFT for the last column (useful for money, file size etc).

Code:

use Symfony\Component\Finder\Finder;
use Symfony\Component\Console\Helper\Table;
use Symfony\Component\Console\Helper\TableStyle;

$table = new Table($output);
$table->setHeaders(['#', 'Path', 'Size']);

$style = new TableStyle();
$style->setPadType(STR_PAD_LEFT);

$table->setColumnStyle(2, $style);

$finder = new Finder();
$finder->files()->in("/path/to/dir")->name('*.php')
$counter = 0;
foreach ($finder as $file) {
    $counter++;
    $table->addRow([$counter, $file->getRealPath(), number_format($file->getSize(), 0, '.', ' ')]);
}

$table->render();

Output:

+---+---------------------+--------+
| # | Path                |   Size |
+---+---------------------+--------+
| 1 | autoload.php        |    183 |
| 2 | ApplicationTest.php | 47 794 |
| 3 | CommandTest.php     | 14 965 |
| 4 | ListCommandTest.php |  2 369 |
+---+---------------------+--------+

{
if (isset($this->columnStyles[$columnIndex])) {
return $this->columnStyles[$columnIndex];
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove else here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aitboudad Nope, all columns should have current table style by default, if not assigned from outside. If I remove else, rendering will be broken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MAXakaWIZARD else is useless as you call return in if... Your code should look like:

        if (isset($this->columnStyles[$columnIndex])) {
            return $this->columnStyles[$columnIndex];
        }

        return $this->getStyle();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stloyd got it

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from e88006a to 07be42a Compare March 25, 2015 09:42
*/
public function setColumnStyle($columnIndex, $name)
{
if (!is_numeric($columnIndex)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be is_int as 0.9 is not a valid column index but is valid for is_numeric

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_int will return false for string '23', for example

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from 7c44129 to efba0a7 Compare March 26, 2015 19:59
@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Any thoughts?

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from efba0a7 to ddd3620 Compare March 26, 2015 21:07
* Sets table style.
*
* @param int $columnIndex Column index
* @param TableStyle|string $name The style name or a TableStyle instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should apply cs fixer here to align doc block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aitboudad done

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from 93d0483 to 9489871 Compare March 26, 2015 21:27
@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Any chances to have this in 2.7?

@fabpot
Copy link
Member

fabpot commented Apr 7, 2015

@MAXakaWIZARD Why do we provide a style for columns and not rows? Would it make sense as well?

@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Well, I described it in first comment. It is useful for numeric data, for example.

@fabpot
Copy link
Member

fabpot commented Oct 5, 2015

👍 ping @symfony/deciders

@weaverryan
Copy link
Member

👍 looks very nice. If we also wanted row styling, we could add that later.

*/
public function setColumnStyle($columnIndex, $name)
{
$isIndexValid = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should assume that $columnIndex is an int, IMO we can remove checking the columnIndex type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Should this PR target version 2.8?

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

2.7 is fine. I will merge it automatically in the 2.8 branch anyway, no need to rebase on current 2.8.


if (!$isIndexValid) {
throw new \InvalidArgumentException(sprintf('Invalid column index: "%s".', $columnIndex));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the lines above can be removed.

@MAXakaWIZARD
Copy link
Contributor Author

@fabpot @stof I've made changes to the code regarding your comments


/**
* Gets the current style for specified column,
* if style was not set, returns global table style
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 prefer something along the lines of:

/**
 * Gets the current style for a column.
 *
 * If style was not set, it returns the global table style.
 */

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Apart from my small phpdoc comment, 👍

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from 1a2a43d to 06f9c51 Compare October 6, 2015 14:48
@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Done

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Can you keep the dots as in my original proposal?

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

In fact, I still prefer my version than yours.

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch 3 times, most recently from 50f0f20 to ad15c25 Compare October 6, 2015 14:55
@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Sorry. Is it OK now?

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Looks good now. Can you rebase on current 2.7 to get rid of the merge commit? A simple git fetch origin && git rebase origin/2.7 from your branch should be enough. Thanks for your patience.

@MAXakaWIZARD MAXakaWIZARD force-pushed the pr-console-table-columns-styles branch from ad15c25 to 0623598 Compare October 6, 2015 15:18
@MAXakaWIZARD
Copy link
Contributor Author

@fabpot Done. It's only one commit finally.

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Thank you @MAXakaWIZARD.

@fabpot fabpot closed this in 21e50d5 Oct 6, 2015
@fabpot fabpot reopened this Oct 6, 2015
@fabpot fabpot closed this Oct 6, 2015
@fabpot fabpot mentioned this pull request Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants