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] Console output formatter improvement#45318
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
carsonbot commentedFeb 5, 2022
Hey! I think@boesing has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
theofidry 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.
A giant 👍 for this. Badly needed!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| /** | ||
| * Simple output wrapper for "tagged outputs" instead of wordwrap(). This solution is based on a StackOverflow | ||
| * answer: https://stackoverflow.com/a/20434776/1476819 from SLN. |
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.
SLN?
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 started to do this 2 years ago, SLN was the original name of theuser557597.
| interface OutputWrapperInterface | ||
| { | ||
| publicconstTAG_OPEN_REGEX ='[a-z](?:[^\\\\<>]*+ |\\\\.)*'; |
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.
| publicconstTAG_OPEN_REGEX ='[a-z](?:[^\\\\<>]*+ |\\\\.)*'; | |
| publicconstTAG_OPEN_REGEX_SEGMENT ='[a-z](?:[^\\\\<>]*+ |\\\\.)*'; |
? I am not sure it's the best terminology either. I'm just a bit concerned that "regex" here feels wrong as, as a user, I would expect a regex to give a valid regex, not a part of it.
| */ | ||
| class OutputWrapperimplements OutputWrapperInterface | ||
| { | ||
| publicconstURL_PATTERN ='https?://\S+'; |
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.
Might be better to make it private.
| if (null !==$type) { | ||
| $type =sprintf('[%s]',$type); | ||
| $indentLength =\strlen($type); | ||
| $indentLength =Helper::width($type); |
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.
It doesn't look directly related to your change no? Asking this in case as it feels like there is a missing test for it and I don't know if your new tests covers it or not
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.
| $output =newSymfonyStyle($input,$output); | ||
| $output->comment( | ||
| 'Lorem ipsum dolor sit <comment>amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.</comment> Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum' | ||
| 'ÁrvíztűrőtükörfúrógépLorem ipsum dolor sit <comment>amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.</comment> Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum' |
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.
maybe a little emoji in the mix? 😄
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 added. Good to know, this is a Hungarian 'artificial word' that contains all of the special characters in the language. In free translation: 'floodproof-mirror-driller'. :D
Uh oh!
There was an error while loading.Please reload this page.
| ornare</error> efficitur. | ||
| EOS; | ||
| $result =$wrapper->wrap($text,20); | ||
| $this->assertEquals($baseExpected,$result); |
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.
might I suggest a provider pattern instead?
/** * @dataProvider textProvider */publicfunctiontestBasicWrap(string$text,int$width,bool$allowCutUrls,string$expected):void {$wrapper =newOutputWrapper();$wrapper->setAllowCutUrls($allowCutUrls);$actual =$wrapper->wrap($text,$width);self::assertSame($expected,$actual);}publicstaticfunctiontextProvider():iterable {yield'title' => [...];}
fc32637 to525e092CompareCo-authored-by: Théo FIDRY <theo.fidry@gmail.com>
Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>
525e092 to8e06868Comparefchris82 commentedFeb 15, 2022
@theofidry Hi, could you review and close the fixed issues? |
Uh oh!
There was an error while loading.Please reload this page.
theofidry commentedFeb 15, 2022
For some reasons I cannot mark them as resolved. LGTM though 👍 |
Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>
fchris82 commentedFeb 15, 2022
None of them? 🤔 |
nicolas-grekas 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.
Here are some random notes :)
Uh oh!
There was an error while loading.Please reload this page.
| * Add method`__toString()` to`InputInterface` | ||
| * Added`OutputWrapperInterface` and`OutputWrapper` to allow modifying your | ||
| wrapping strategy in`SymfonyStyle` or in other`OutputStyle`. Eg: you can | ||
| switch off to wrap URLs. |
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'm not convinced we need to open for extensibility here. Fight me if you want to keep it :)
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 added this because the behavior you need can depend on your console, your language, and other things. I added a really basic solution here, but people may want different behavior, or they want to split the long URL-s on their way. Eg: originally I just wanted to solve the text wrapping but it turned out, URL wrapping is another issue.
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 agree with Nicolas, we should not make it extensible.
| // opening tag? | ||
| if ($open ='/' !=$text[1]) { | ||
| if ($open =('/' !==$text[1])) { |
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.
no need for the brackets
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 added it because PHPStorm recommended this change, and I agree, it is more readable what is happening, even if you are totally right, the original version worked. If you want, I can 'undo' this, up to you. But now you know why I did it.
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 revert as it's not needed.
| publicfunctionwrap(string$text,int$width,string$break ="\n"):string | ||
| { | ||
| if (0 ===$width) { |
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.
if (!$width) {
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.
Done
| * Add method`__toString()` to`InputInterface` | ||
| * Added`OutputWrapperInterface` and`OutputWrapper` to allow modifying your | ||
| wrapping strategy in`SymfonyStyle` or in other`OutputStyle`. Eg: you can | ||
| switch off to wrap URLs. |
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 agree with Nicolas, we should not make it extensible.
| // opening tag? | ||
| if ($open ='/' !=$text[1]) { | ||
| if ($open =('/' !==$text[1])) { |
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 revert as it's not needed.
| namespaceSymfony\Component\Console\Helper; | ||
| interface OutputWrapperInterface |
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 remove this interface.
| privateOutputWrapperInterface$outputWrapper; | ||
| publicfunction__construct(InputInterface$input,OutputInterface$output) | ||
| publicfunction__construct(InputInterface$input,OutputInterface$output,OutputWrapperInterface$outputWrapper =null) |
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 remove this argument.
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.
If the argument and the setters are removed, the propertyOutputWrapper::$allowCutUrls cannot be updated and becomes useless.
| $this->outputWrapper =$outputWrapper; | ||
| return$this; | ||
| } |
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 can also remove these gettter/setter methods.
GromNaN commentedJul 26, 2022
fabpot commentedJul 26, 2022
Closing in favor of#47062 then. |
… (fchris82, GromNaN)This PR was merged into the 6.2 branch.Discussion----------[Console] Don't cut Urls wrapped in SymfonyStyle block| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets |#30920| License | MIT| Doc PR |symfony/symfony-docs#16476Continuation of#45318 by fchris82Commits-------b317f06 Remove OutputWrapperInterface2bc2571 Console output formatter improvement
…utput wrapper (fchris82)This PR was submitted for the 6.1 branch but it was merged into the 6.2 branch instead.Discussion----------[Console] Add note about URL cutting of default output wrapperAdd note about the new output wrapper function. Related MR:symfony/symfony#45318Commits-------0bfa8ac [Console] Add note about URL cutting of default output wrapper
Current
SymfonyStylecan cause some problems:I fixed the problem and added or modified some tests. This is a simple, fast, and working solution with a regular expression that won't wrap in the middle of a formatting tag or a link. The last one is optional, see the doc change.