Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Support max column width in Table#28373
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $lines =array(); | ||
| foreach (explode("\n",$text)as$line) { | ||
| $lines[] ='' ===$line ?$line :$this->styleStack->getCurrent()->apply($line); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
here we fix#27832 by formatting each line separate. This only helps when the user wraps a string. If it's wrapped by the terminal it can still occur.
nicolas-grekas commentedSep 6, 2018
(tests are red) |
ro0NL commentedSep 6, 2018
Ah forgot to mention; failures unrelated :) fabbot.io is a false-positive IMHO travis appveyor: |
nicolas-grekas commentedSep 6, 2018
I restarted the builds on Travis and appveyor. Let's fix fabbot. |
ro0NL commentedSep 6, 2018
Done. Consistent with |
| if ('' !==$current &&"\n" !==substr($current, -1)) { | ||
| $text ="\n".$text; | ||
| if ($this->isDecorated()) { | ||
| foreach ($linesas &$line) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's not use a reference when we can do without
| * | ||
| * @return $this | ||
| */ | ||
| publicfunctionsetColumnMaxWidths(array$widths):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we really need this one? Can't we do the same with setColumnMaxWidth and keep a smaller public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
currently we havesetColumnWidth() andsetColumnWidths() to control min. width. We can do withoutsetColumnMaxWidths() but might raise some questions (why cant i set max. widths at once?)
IMHO the ideal api is:setColumnMinWidth + setColumnMaxWidth + setColumnWidth where the latter sets both min. and max. (thus fixed). However that requires to rename the currentsetColumnWidth tosetColumnMinWidth first. Not aiming for that :)
Do you suggest to dropsetColumnMaxWidths and add array support insetColumnMaxWidth or just drop it?
ro0NL commentedSep 7, 2018
Ready for me :) really happy with this feature. |
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like this! Thanks Roland.
| $offset =0; | ||
| $output =''; | ||
| $tagRegex ='[a-z][a-z0-9,_=;-]*+'; | ||
| $length =0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor comment: should we rename$length as$maxLength ? "length" is too generic, and in this case is confusing because we are also using "width" -->$this->applyCurrentStyle(..., ..., $width, $length)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$currentLineLength? It's not a max value, e.g. for$width = 5 and$text = '123456'
12345 // $length = 0 aka "full"6 // $length = 1So we know we can add 5-1=4 more chars before the next line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 for$currentLineLength
Uh oh!
There was an error while loading.Please reload this page.
This PR was merged into the 3.4 branch.Discussion----------[Console] Fix typo in tests| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Spotted in#28373cc@chalasrCommits-------01e491e [Console] Fix typo in tests
chalasr commentedSep 11, 2018
Thank you@ro0NL. |
This PR was squashed before being merged into the 4.2-dev branch (closes#28373).Discussion----------[Console] Support max column width in Table| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#22156,#27832| License | MIT| Doc PR |symfony/symfony-docs#10300Continuation of#22225 to better preserve spaces (which preserves background colors), using `wordwrap` it caused some issues.Also the wrapping was plain wrong by not taking the current line length into account.While at it, it comes with `Table` integration :)Given```php$table = new Table($output);$table->setColumnMaxWidth(0, 2);$table->setRow(0, ['pre <error>foo bar baz</error> post']);$table->render();$table = new Table($output);$table->setColumnMaxWidth(0, 3);$table->setRow(0, ['pre <error>foo bar baz</error> post']);$table->render();$table = new Table($output);$table->setColumnMaxWidth(0, 4);$table->setRow(0, ['pre <error>foo bar baz</error> post']);$table->render();```Commits-------175f68f [Console] Support max column width in Table
pjcdawkins commentedNov 30, 2018
In case you're interested in an older implementation of something that sets column widths according to the terminal width: Looks like it can now be significantly simplified based on these Console changes (thanks@ro0NL) |
Uh oh!
There was an error while loading.Please reload this page.
Continuation of#22225 to better preserve spaces (which preserves background colors), using
wordwrapit caused some issues.Also the wrapping was plain wrong by not taking the current line length into account.
While at it, it comes with
Tableintegration :)Given