-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] fix emojis messing up the line width #40524
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
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
Do we really need a new method? Can't we update |
a08f6cd
to
da6fcf5
Compare
Hello @chalasr, thanks for the feedback. Since the method is used in 13 other places in the code, I was afraid it would break something. But after trying it and running the tests, it seems it doesn't 👍 |
add tests + removed irrelevant method
da6fcf5
to
36b36dc
Compare
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 applied my own review comment and fixed a few more things found meanwhile)
Thank you @MarionLeHerisson. |
I like this feature, however, it caused some regressions. I will revert this an add it as a new feature in 5.3 instead. See #40697 |
symfony/symfony#40524 will be investigated and implemented in future Termage releases
Description
The emojis, because they take as much space as two characters, would cause the console to display too many spaces to complete a line, which made it uneven, as described in the issue.
The fix uses the
width
function instead ofstrlen
. To answer @ogizanagi's comment, yes it does work with "composed" emojis.Before :
After :
Other changes
Removed two unused lines of code, the value of
$messageLineLength
was never used.Note
I'd like to add some tests, but I don't know how since I think this depends on console client width ?
Thanks for your reviews 🙏