[WIP] [Console] Refactored console formatting #31131

fchris82 · 2019-04-16T13:45:09Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	yes
BC breaks?	yes
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#30519, #30920
License	MIT
Doc PR	TODO

Antecedent:

I made a full refactoring. The original output formatting isn't elegant and hard to extend. I made a lexer with tokens and visitors. This solution is based on Design Patterns: Elements of Reusable Object-Oriented Software book, from Erich Gamma, Richard Helm, Ralph Johnson, John M Vlissides (Chapter 2)

TODO:

Extend Upgrade.md
Create Doc PR
Discussion

fchris82 · 2019-04-16T13:48:43Z

@ro0NL Hello again :) I'm waiting for your observations. Yes, it became huge. :-/

stof · 2019-04-16T13:48:02Z

src/Symfony/Component/Console/CHANGELOG.md

@@ -6,6 +6,13 @@ CHANGELOG

 * added support for hyperlinks
 * added `ProgressBar::iterate()` method that simplify updating the progress bar when iterating
+ * refactored console output formatter: add lexer and tokens. Important changes:
+    * added `<wrap>` and `<nowrap>` formatter tags to console
+    * [BC BREAK] earlier the unhandled/unknown tags will be shown, but now every tag will disappear after formatting. If you want to show a "tag", you have to escape it with `\` sign: `\<visibleTag>...\</visibleTag>`. You can use the `\Symfony\Component\Console\Formatter\OutputFormatter::escape()` function as well.


BC breaks are a no-go in a minor version. This must trigger a deprecation instead to provide a migration path.

stof · 2019-04-16T13:48:56Z

src/Symfony/Component/Console/CHANGELOG.md

+ * refactored console output formatter: add lexer and tokens. Important changes:
+    * added `<wrap>` and `<nowrap>` formatter tags to console
+    * [BC BREAK] earlier the unhandled/unknown tags will be shown, but now every tag will disappear after formatting. If you want to show a "tag", you have to escape it with `\` sign: `\<visibleTag>...\</visibleTag>`. You can use the `\Symfony\Component\Console\Formatter\OutputFormatter::escape()` function as well.
+    * [BC BREAK] `\Symfony\Component\Console\Formatter\OutputFormatterStyleInterface` was changed: `apply()` function was removed, and there were set 2 new methods: `start()`, `close()`


no-go. This must use deprecations, and adding new methods in an interface is also a no-go (use a separate interface maybe ?)

stof · 2019-04-16T13:49:20Z

src/Symfony/Component/Console/CHANGELOG.md

+    * added `<wrap>` and `<nowrap>` formatter tags to console
+    * [BC BREAK] earlier the unhandled/unknown tags will be shown, but now every tag will disappear after formatting. If you want to show a "tag", you have to escape it with `\` sign: `\<visibleTag>...\</visibleTag>`. You can use the `\Symfony\Component\Console\Formatter\OutputFormatter::escape()` function as well.
+    * [BC BREAK] `\Symfony\Component\Console\Formatter\OutputFormatterStyleInterface` was changed: `apply()` function was removed, and there were set 2 new methods: `start()`, `close()`
+    * [BC BREAK] `\Symfony\Component\Console\Formatter\OutputFormatter` is no longer instance of `WrappableOutputFormatterInterface`


that BC break is also a no-go.

stof · 2019-04-16T13:50:47Z

src/Symfony/Component/Console/Formatter/Lexer.php

+class Lexer implements LexerInterface
+{
+    /** @var FullTextToken */
+    protected $fullTextToken;


everything must use private visibility, not protected one, unless we explicitly decide to make inheritance the supported extension point (hint: such decision is always the last choice).

stof · 2019-04-16T13:51:35Z

src/Symfony/Component/Console/Formatter/LexerInterface.php

+     *
+     * @param string $text
+     *
+     * @return \IteratorAggregate


should return \Traversable or iterable. There is no reason to enforce that \IteratorAggregate is used to implementator the iterator.

stof · 2019-04-16T13:53:34Z

src/Symfony/Component/Console/Formatter/OutputFormatterStyleStack.php

    {
-        $this->styles[] = $style;
+        $this->styles[$depth] = $style;


this is not pushing anything anymore. It replaces an element of the stack

stof · 2019-04-16T13:54:33Z

src/Symfony/Component/Console/Formatter/Tokens/EosToken.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Console\Formatter\Tokens;


our namespaces are singular

stof · 2019-04-16T13:55:15Z

src/Symfony/Component/Console/Tests/Formatter/OutputFormatterTest.php

@@ -15,6 +15,10 @@
 use Symfony\Component\Console\Formatter\OutputFormatter;
 use Symfony\Component\Console\Formatter\OutputFormatterStyle;

+/**
+ * @author Fabien Potencier <fabien@symfony.com>
+ * @author Krisztián Ferenczi <ferenczi.krisztian@gmail.com>


We don't put @author phpdoc on test files.

stof · 2019-04-16T13:56:10Z

src/Symfony/Component/Console/Tests/Formatter/OutputFormatterTest.php

        $style = new OutputFormatterStyle('blue', 'white');
-        $formatter->setStyle('test', $style);


this test should be kept unchanged. If you cannot, it means there is a BC break. And that's not something we want

stof · 2019-04-16T13:57:21Z

src/Symfony/Component/Console/Tests/Formatter/OutputFormatterTest.php

@@ -318,26 +323,13 @@ public function testContentWithLineBreaks()
        ));
    }

-    public function testFormatAndWrap()


this test must be kept.

stof · 2019-04-16T13:58:57Z

Can we also get a performance comparison of the old and new system ?

fchris82 · 2019-04-16T14:10:22Z

Can we also get a performance comparison of the old and new system ?

I will make one. I think the new is worse, but I thought this is the console, we don't want to format large texts/outputs, and the time isn't so important on the console. I inserted a limitation here: https://github.com/symfony/symfony/pull/31131/files#diff-a42222f449d2d525a1100ed2b995e429R43 on the bases of this: http://piotrpasich.com/spl-iterators-against-the-performance/ (first comment: http://piotrpasich.com/spl-iterators-against-the-performance/#comment-1818526630 )

fchris82 · 2019-04-16T15:25:02Z

@stof I wrote a benchmark: https://gist.github.com/fchris82/e09e4d9723e4b0e73a768ab53860d321 and I ran it:

1000x original
--------------
Short           0.03s
Short with tags 0.29s
Simple          0.03s
With tags       1.56s

100x new
--------
Short           1.02s
Short with tags 1.24s
Simple          44.41s
With tags       47.42s

Yes, there are big differences. Maybe should I keep the original Formatter and create a new one for wrapping and other actions?

fchris82 · 2019-04-18T10:26:42Z

@stof Hello, I made a twice faster solution with plain array token container: fchris82@d4fc1d2

$tokens = [
    [Lexer::TYPE_WORD, 'Lorem'],
    [Lexer::TYPE_SEPARATOR, ' '],
    [Lexer::TYPE_WORD, 'ipsum'],
    // ...
    new FullTokenTag('<comment>'),
    // ...
];

This solution is faster, but some things are lost in this case, eg: type hinting (there are strings and Token objects mixed in "one array") What do you prefer?

My opinion: I kept the original OutputFormatter also, so if somebody want to format anything fast (eg: large text and wrapping insn't important), original can be used for them. In most of case we use short texts on console, "twice time" doesn't matter.

stof · 2019-04-18T12:53:20Z

why does your benchmark result have 1000x original and 100x new ? We need comparable data. If I understand things properly, the old code can format 1000 messages in 30ms, meaning 30µs per message, while the new code formats 100 messages in 1s, meaning 10000µs per message. That's not twice slower. That's 300 times slower.

fchris82 · 2019-04-19T08:36:12Z

@stof You misunderstood my last comment. There are more problems with the word wrapping in SymfonyStyle now:

tag wrapping
UTF-8 chars
wrapping URL-s

There are 3 different "solution" now:

A fast fix: [Console] Upgrade word wrapping in SymfonyStyle #30519 and [WIP] [Console] Pretty word wrapping #30590 ("speed": ~1-2x current)
"this" ("speed": 300x current)
like this, just a little bit optimazed: fchris82@d4fc1d2 <-- an other branch! ("speed": 150x current)

Yes, this solution is much slower, but works, it wraps the text well, easy to extend, and my question is: does this speed matter on console? Is there any different than the full text is shown 0.03 s instead of 0.0001 s on the screen? Important: we are talking about console, we use here shorter texts. (What I used in the performance test file, it is a long lorem ipsum text, we almost never print like this) If you think, that is too slow, I will finish the first version. If you think, this can be acceptable, I will finish this version.

I would like to solve the problem, I don't insist on this version, only I think this is the "best" version, with an reasonable speed "problem".

fchris82 · 2019-04-24T13:45:46Z

@stof Please give me a direction, what do you prefer:

fast with double "parsing"
slow but easy to extend

carsonbot added Status: Needs Review Console Bug Feature BC Break Deprecation labels Apr 16, 2019

stof reviewed Apr 16, 2019

View reviewed changes

nicolas-grekas added this to the next milestone Apr 17, 2019

fchris82 closed this Aug 29, 2019

fchris82 force-pushed the master branch from 90b1a70 to 8d96861 Compare August 29, 2019 07:04

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

		$style = new OutputFormatterStyle('blue', 'white');
		$formatter->setStyle('test', $style);

Uh oh!

[WIP] [Console] Refactored console formatting #31131

[WIP] [Console] Refactored console formatting #31131

Uh oh!

Conversation

fchris82 commented Apr 16, 2019

Uh oh!

fchris82 commented Apr 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 16, 2019

Uh oh!

fchris82 commented Apr 16, 2019

Uh oh!

fchris82 commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fchris82 commented Apr 18, 2019

Uh oh!

stof commented Apr 18, 2019

Uh oh!

fchris82 commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fchris82 commented Apr 24, 2019

Uh oh!

Uh oh!

fchris82 commented Apr 16, 2019 •

edited

Loading

fchris82 commented Apr 19, 2019 •

edited

Loading