-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
5.1 - Restore command helper tests and add BannerHelper #17686
Conversation
This was lost in the 4.x -> 5.x changes.
It was lost in the 4.x -> 5.x updates.
src/Console/ConsoleOutput.php
Outdated
'info' => ['text' => 'cyan'], | ||
'info.bg' => ['background' => 'cyan', 'text' => 'black'], |
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.
A bright color for info would look like a result to me. Could we try grey with cyan text?
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.
We don't have grey right now, as we only do the basic 8 color ansi palette. We could use white
as the background, or not include a banner style for info 🤷
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.
White would work.
|
||
$lines = [ | ||
'', | ||
$start . str_repeat(' ', $bannerLength) . $end, |
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.
We are always assuming utf-8 for multi-byte encoding, right?
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.
Yeah, if folks need a non-utf8 encoding they'll have to configure the global encoding.
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.
Right. What I meant was we're injecting ascii characters into an mb stream, so we can only support utf8 output.
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.
Just realized this is only for console. I don't think consoles support anything incompatible with ascii space.
- newer phpunit eats our deprecation warnings. - Fix phpcs error and phpstan config warning - Fix new type issues that phpstan found - Update split repos phpstan config
A cyan background could be mistaken for green.
When adding
BannerHelper
I noticed that we were missing test cases for the other helpers. It seems like the files were removed during 5.x, I have to assume during one of the many very large merge conflicts. I've restored those tests now.These changes also contain a new console helper for emitting banners. With my terminal theme the following code
Emits the following.
I would appreciate any other results, as I'm not sure I have the colors right.