Refactor FeatureComplete code for formatting help text #189
Conversation
This commit moves the code related to checking color and formatting the help text to a new helper class. This way it can be reused when the new DocCodeExamples script is introduced. Tests were added for the new HelpTextFormatter::format() method.
Scripts/Utils/HelpTextFormatter.php Outdated
| * | ||
| * @return bool | ||
| */ | ||
| public static function isColorSupported() |
There was a problem hiding this comment.
I don't think this method belongs in the HelpTextFormatter class as this is also used to determine whether the non-help output of the CLI command(s) should be colorized or not...
I have a feeling this should be in a separate class, which maybe should also contain some "colorize" helper methods.
There was a problem hiding this comment.
As we discussed in a call, I will update this PR to remove isColorSupported() and then look into potentially creating a separate PR to add https://github.com/php-parallel-lint/PHP-Console-Color/ as a dependency and use it in this package.
jrfnl added Type: enhancement Status: awaiting feedback Type: chores/QA and removed Type: enhancement labels Partially reverts the previous commit to keep isColorSupported() on the Config class as discussed during the PR review.
rodrigoprimo force-pushed the refactor-feature-complete-config-class branch from 9b72325 to 96a0c87 Compare jrfnl changed the title There was a problem hiding this comment.
Thanks for setting this up @rodrigoprimo !
A couple of questions about the "design":
- Is the
PHPCSDevTools\Scripts\FeatureComplete\Config::getHelp()method still necessary ? - Any particular reason why the
HelpTextFormatteris a static class and not an instantiatable one ? - As the logic for the help display has now moved to the
HelpTextFormatter, I wonder how much value the tests in theTests\FeatureComplete\GetHelpTestnow have and whether the@coversfor that test is still correct/useful.
| /** | ||
| * Format help text from a structured array of options. | ||
| * | ||
| * @param array<string, array<array<string, string>>> $helpTexts The help texts to format. |
There was a problem hiding this comment.
As the formatter is now in a separate class, I think the expected format for the $helpTexts array needs to be documented in more detail.
The class docblock might even be a better place than this function docblock.
2 participants
This PR moves the code related to checking color and formatting the help text to a new helper class. This way, it can be reused when the new DocCodeExamples script is introduced.
Tests were added for the new HelpTextFormatter::format() method.
This commit was originally added to #185, but after discussing it with @jrfnl, I'm opening a separate PR for it.