Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

maidmaid
Copy link
Contributor

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 /

@maidmaid maidmaid changed the title [Console] Set box style table as default [WIP][Console] Set box style table as default Mar 19, 2018
@javiereguiluz javiereguiluz added Feature Console ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" labels Mar 19, 2018
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 19, 2018
nicolas-grekas
nicolas-grekas previously approved these changes Mar 19, 2018
View reviewed changes
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 19, 2018 •
edited
Loading

Can anyone actually try on Windows? Because here, this is UTF-8, but these characters require CP437 on Windows, which we do not output AFAIK.

@nicolas-grekas nicolas-grekas dismissed their stale review March 19, 2018 12:52

to be confirmed on Windows

@ogizanagi
Copy link
Contributor

I thought it was already tested on Windows. But indeed, a confirmation would be great.

nicolas-grekas
nicolas-grekas requested changes Mar 21, 2018
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I confirm unicode characters are not supported on cmd.com. It works with the xterm one. What could work on both Linux and Windows is using box when preg_match('/utf-?8/i', getenv('LANG')).

src/Symfony/Component/Console/Helper/Table.php
@@ -130,6 +130,10 @@ public static function getStyleDefinition($name)
*/
public function setStyle($name)
{
if ('symfony-style-guide' === $name) {
@trigger_error(sprintf('Style symfony-style-guide is deprecated since Symfony 4.1.', __METHOD__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

The "box" style doesn't work on Windows cmd.com terminal, so we cannot deprecate symfony-style-guide.

@maidmaid
Copy link
Contributor Author

@nicolas-grekas The most similar style to box style is the default, not the symfony-style-guide (see below). So, when box style can't work properly as you have explained, why not use the default style which looks like the most consistent workaround, and thus deprecate symfony-style-guide?

box
┌───────────────┬──────────────────────────┬──────────────────┐
│ 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  │
└───────────────┴──────────────────────────┴──────────────────┘

default
+---------------+--------------------------+------------------+
| 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  |
+---------------+--------------------------+------------------+

symfony-style-guide
 --------------- -------------------------- ------------------
  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
 --------------- -------------------------- ------------------

@nicolas-grekas
Copy link
Member

No need to deprecate IMHO, the symfony-style-guide is still visually nice.

@fabpot
Copy link
Member

fabpot commented Mar 22, 2018

Ah, I didn't realize that the default Symfony theme is not the console default. I would say that we don't need to change anything then. The current default looks great as is and works everywhere. Sorry for asking for a change, I should have checked what we were using first.

@maidmaid
Copy link
Contributor Author

IMO, the box style allows the best readability of a table thanks to full lines. It would be nice to use it when possible (as suggested by @nicolas-grekas).

My new suggestion is to use the box style when possible, and use symfony-style-guide as a workaround when this first one can't work.

@fabpot
Copy link
Member

fabpot commented Mar 25, 2018

I like consistency. So, I'd recommend to close this PR. Sorry for making you work on this one. My mistake.

@chalasr
Copy link
Member

chalasr commented Mar 28, 2018 •
edited
Loading

👍 for closing

@maidmaid maidmaid closed this Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@ogizanagi ogizanagi ogizanagi approved these changes

Assignees
No one assigned
Labels
Console Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Needs Work
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
7 participants
@maidmaid @nicolas-grekas @ogizanagi @fabpot @chalasr @javiereguiluz @carsonbot