8000 [Console] Fix line wrapping for decorated text in block output by grasmash · Pull Request #40348 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] Fix line wrapping for decorated text in block output #40348

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
Merged
merged 1 commit into from
Mar 17, 2021
Merged

[Console] Fix line wrapping for decorated text in block output #40348

merged 1 commit into from
Mar 17, 2021

Conversation

grasmash
Copy link
Contributor
@grasmash grasmash commented Mar 2, 2021
Q A
Branch?
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Fixed bug that caused decorated text to be wrapped too early in SymfonyStyle->block().

@grasmash grasmash requested a review from chalasr as a code owner March 2, 2021 21:31
@carsonbot carsonbot added this to the 4.4 milestone Mar 2, 2021
@grasmash
Copy link
Contributor Author
grasmash commented Mar 3, 2021

Quick before and after:

- // Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et             \n
- // dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea       \n
- // commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla     \n
- // pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim            \n
- // id est laborum                                                                                                      \n
+ // Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore      \n
+ // magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo      \n
+ // consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla             \n
+ // pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id         \n
+ // est laborum                                                                                                         \n

Notice that after, we are correctly filling the line with text up to 120 chars as expected. Before, the words were wrapped too early.

@carsonbot
Copy link

Hey!

I like what you have done here. Keep up the good work.

I think @Simperfit has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@stof
Copy link
Member
stof commented Mar 3, 2021

what if there is extra decoration inside the message ?

@grasmash grasmash closed this Mar 11, 2021
@grasmash grasmash reopened this Mar 15, 2021
@grasmash
Copy link
Contributor Author

@stof

what if there is extra decoration inside the message ?

Not sure what you mean. We're measuring the "decoration" with Helper::strlen($message) - Helper::strlenWithoutDecoration($this->getFormatter(), $message);. What other decoration might there be?

grasmash
grasmash commented Mar 15, 2021
View reviewed changes
@nicolas-grekas nicolas-grekas changed the title Correct line wrapping for decorated text in block output. [Console] Correct line wrapping for decorated text in block output. Mar 16, 2021
nicolas-grekas
nicolas-grekas reviewed Mar 16, 2021
View reviewed changes
@grasmash
Copy link
Contributor Author

I'd like to note that while this PR improves the situation, it's still not perfect. The correct solution would actually be to follow the example of formatAndWrap() and actually parse the string one character at a time to wrap the line while not breaking a tag.

I looked into using formatAndWrap(), but it's problematic. We really need the wrap aspect of it and not the format aspect.

@grasmash grasmash force-pushed the patch-3 branch 2 times, most recently from ff051ea to ca1753b Compare March 16, 2021 15:02
@carsonbot carsonbot changed the title [Console] Correct line wrapping for decorated text in block output. Correct line wrapping for decorated text in block output. Mar 17, 2021
@nicolas-grekas nicolas-grekas changed the title Correct line wrapping for decorated text in block output. [Console] Fix line wrapping for decorated text in block output Mar 17, 2021
@nicolas-grekas
Copy link
Member

Thank you @grasmash.

@nicolas-grekas nicolas-grekas merged commit 9030fd3 into symfony:4.4 Mar 17, 2021
This was referenced Mar 29, 2021
Merged
Merged
@greg0ire greg0ire mentioned this pull request Mar 30, 2021
Closed
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Mar 30, 2021
@greg0ire
Test against correct wrapping behavior
7be4b4a 
Decorated text used to be wrapped too early in SymfonyStyle->block()
See symfony/symfony#40348
@greg0ire greg0ire mentioned this pull request Mar 30, 2021
Merged
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Mar 30, 2021
@greg0ire
Accommodate 2 behaviors of symfony/console in test
fe49640 
Decorated text used to be wrapped too early in SymfonyStyle->block()
See symfony/symfony#40348
The fix was not contributed to version 3, which means we have to rewrite
the test so that it passes for both the correct and the buggy version.
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 approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@grasmash @carsonbot @stof @nicolas-grekas
0