[Console] Enhance MarkdownDescriptor #20896

ogizanagi · 2016-12-13T13:14:10Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

In order to add usefulness to this descriptor, this PR allows to jump to a command description by clicking on the command name on the application summary.

It's not a generic approach but IMHO a quick win that should work at least for Github flavoured markdown.

The markdown format should also avoid using Application::getLongVersion as the markdown document title, as it may contain tags (and unnecessary information using the framework).

Lastly, the markdown format is now cleaned from tags initially used for console output formatting and only keeps properly escaped ones.

fabpot · 2016-12-13T15:34:58Z

@ogizanagi That's too many PRs on the same subject. Too much overhead for everyone. I'm going to close the 2 other ones. Let's do everything here. Let's really think about this and let's say that this one is the last one on the topic.

ogizanagi · 2016-12-13T15:52:43Z

No problem with that. I've cherry-picked 90dee8c so we can discuss about everything here.

Last issue that should be tackled (IMHO in a dedicated PR unless you consider it a new feature rather than a bug fix for 3.2) is your comment about json & xml descriptors that should show hidden commands too.

ro0NL · 2016-12-13T17:07:51Z

Tags are already included in help texts as well. Hence i chose to use getLongVersion as well. Tags are not shown anyway..

ogizanagi · 2016-12-13T18:15:11Z

Tags are not shown anyway

@ro0NL: That's not true when using shortcuts (i.e <info>3.0</>) as Symfony\Bundle\FrameworkBundle\Console\Application::getLongVersion() does.

We can't do much for help texts, but this is the markdown document title. It also displays information that should not been shown for the format purpose (kernel, env, debug).

ro0NL · 2016-12-13T18:19:38Z

I used github to preview, ie. the title in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Tests/Fixtures/application_2.md looks right to me

ogizanagi · 2016-12-13T18:26:02Z

Here is what it looks like with tags shortcuts and the Symfony\Bundle\FrameworkBundle\Console\Application::getLongVersion(): https://github.com/ogizanagi/symfony/blob/8be2185a778c04dc87fb3f9e62d2e1ce31ed3066/application.md

Anyone may have used short closing tags in their own application and getLongVersion() implementation.

ro0NL · 2016-12-13T18:51:51Z

Fair enough. Let's add a test for it then :)

edit: what if we just filter text with tags usingstrip_tags. Wouldnt that be much simpler?

edit2: https://3v4l.org/3DFsX

ogizanagi · 2016-12-26T15:24:39Z

@ro0NL : Sorry, I've missed your comment.

Fair enough. Let's add a test for it then :)

Which kind of test do you have in mind? IMHO it isn't worth adding a test related to this for the FrameworkBundle Application class. Anyway, we don't use getLongVersion anymore here, so there is nothing to test regarding the mentioned tags and extra info previously displayed.

what if we just filter text with tags usingstrip_tags. Wouldnt that be much simpler?

If you mean just filtering the result of getLongVersion to keep using it, yes, but the fact is getLongVersion can still return anything not suitable for a document title. Pragmatically, we just need the application title. So there is no reason to use getLongVersion over directly using what we really need.

If you mean stripping all tags in every texts, the issue is that striptags won't preserve tags that might be legit in a description, even when escaping it, on the contrary of calling OutputFormatter::format from a non decorated instance.
I can update this PR to disable decoration for the markdown descriptor and calling Descriptor::write() with the $decorated argument ironically set to true 😅 in order to escape any text while preserving explicitly escaped tags.

ro0NL · 2016-12-26T15:44:03Z

Maybe use short notation in the component, ie here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Application.php#L342

That should cover.

but the fact is getLongVersion can still return anything not suitable for a document title

Guess your right.. it looks bad if long version is something like

<name> <version>
Some built date notice
Some copyright notice

What about UX-wise, the following header;

<app> <version> # or Console Tool by default
===============

\```
<long-version> # not shown if same as default?
\```

# etc.

ro0NL · 2016-12-26T15:48:35Z

Or lets forget about getLongVersion.. not sure the info is worth it anyway?

Lets do the XML/JSON approach 👍 and continue hardcoding UNKNOWN (sorry couldnt resist ;-))

Status: Reviewed

ogizanagi · 2016-12-26T17:28:51Z

In 7687bf5, I've added what I mentioned in #20896 (comment)

I can update this PR to disable decoration for the markdown descriptor and calling Descriptor::write() with the $decorated argument ironically set to true 😅 in order to escape any text while preserving explicitly escaped tags.

Status: Needs review

ogizanagi · 2016-12-26T17:30:05Z

src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php

+    /**
+     * {@inheritdoc}
+     */
+    protected function write($content, $decorated = true)


Don't know if it's something acceptable to change the default value of an argument ($decorated) when overriding a method for such case.

Default makes sense i guess. Is it really worth against changing write calls?

ogizanagi · 2016-12-29T18:39:11Z

I don't have more changes to bring to the descriptor. IMHO this PR is ready for a decision.

ro0NL

👍

ro0NL · 2016-12-29T18:43:38Z

src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php

@@ -142,4 +168,17 @@ protected function describeApplication(Application $application, array $options
            $this->write($this->describeCommand($command));
        }
    }
+
+    private function getApplicationTitle(Application $application)


If we do a new method i'd say ApplicationDescription::getTitle for better reuseability.

ro0NL · 2016-12-29T18:46:06Z

src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php

+    /**
+     * {@inheritdoc}
+     */
+    protected function write($content, $decorated = true)


Default makes sense i guess. Is it really worth against changing write calls?

ro0NL · 2016-12-29T18:49:09Z

src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php

@@ -102,7 +125,7 @@ protected function describeCommand(Command $command, array $options = array())
            .array_reduce(array_merge(array($command->getSynopsis()), $command->getAliases(), $command->getUsages()), function ($carry, $usage) {
                return $carry.'* `'.$usage.'`'."\n";
            })
-        );
+        , true);


Can be removed.

Thank you :)

ro0NL · 2016-12-29T18:50:42Z

src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php

-            $this->write(implode("\n", array_map(function ($commandName) {
-                return '* `'.$commandName.'`';
+            $this->write(implode("\n", array_map(function ($commandName) use ($description) {
+                $anchor = '#'.strtr($description->getCommand($commandName)->getName(), array(':' => ''));


Maybe do simple str_replace here. And this can actually be $commandName right? 😕

This allows to handle aliases. See https://github.com/symfony/symfony/pull/20896/files#diff-57586bcf27971e696e76fbcec2e17283R4 (the anchor generated for the alias)

fabpot · 2017-02-16T14:19:31Z

Thank you @ogizanagi.

This PR was squashed before being merged into the 3.3-dev branch (closes #20896). Discussion ---------- [Console] Enhance MarkdownDescriptor | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A In order to add usefulness to this descriptor, this PR allows to jump to a command description by clicking on the command name on the application summary. It's not a generic approach but IMHO a quick win that should work at least for Github flavoured markdown. The markdown format should also avoid using `Application::getLongVersion` as the markdown document title, as it may contain tags (and unnecessary information using the framework). Lastly, the markdown format is now cleaned from tags initially used for console output formatting and only keeps properly escaped ones. Commits ------- ef5b087 Do not output formatting tags 6896865 [Console] MarkdownDescriptor: Don't use getLongVersion fc2395e [Console] MarkdownDescriptor: Link to commands anchors

carsonbot added Status: Needs Review Console Feature labels Dec 13, 2016

nicolas-grekas modified the milestone: 3.x Dec 13, 2016

This was referenced Dec 13, 2016

[Console] MarkdownDescriptor: Don't use getLongVersion #20903

Closed

[Console] MarkdownDescriptor: Strip tags in app headers #20889

Closed

fabpot closed this Dec 13, 2016

fabpot reopened this Dec 13, 2016

ogizanagi changed the title ~~[Console] MarkdownDescriptor: Link to commands anchors~~ [Console] Enhance MarkdownDescriptor Dec 13, 2016

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 26, 2016

carsonbot added Status: Needs Review and removed Status: Reviewed labels Dec 26, 2016

ogizanagi commented Dec 26, 2016

View reviewed changes

ro0NL reviewed Dec 29, 2016

View reviewed changes

ogizanagi added 2 commits January 21, 2017 11:06

[Console] MarkdownDescriptor: Link to commands anchors

fc2395e

[Console] MarkdownDescriptor: Don't use getLongVersion

6896865

Do not output formatting tags

ef5b087

fabpot merged commit ef5b087 into symfony:master Feb 16, 2017

ogizanagi deleted the feature/console/md_command_links branch February 16, 2017 14:20

nicolas-grekas modified the milestone: 3.x Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Enhance MarkdownDescriptor #20896

[Console] Enhance MarkdownDescriptor #20896

ogizanagi commented Dec 13, 2016 •

edited

Loading

fabpot commented Dec 13, 2016

ogizanagi commented Dec 13, 2016 •

edited

Loading

ro0NL commented Dec 13, 2016

ogizanagi commented Dec 13, 2016

ro0NL commented Dec 13, 2016

ogizanagi commented Dec 13, 2016 •

edited

Loading

ro0NL commented Dec 13, 2016 •

edited

Loading

ogizanagi commented Dec 26, 2016

ro0NL commented Dec 26, 2016 •

edited

Loading

ro0NL commented Dec 26, 2016 •

edited

Loading

ogizanagi commented Dec 26, 2016

ogizanagi Dec 26, 2016 •

edited

Loading

ro0NL Dec 29, 2016

ogizanagi commented Dec 29, 2016

ro0NL left a comment

ro0NL Dec 29, 2016

ro0NL Dec 29, 2016

ro0NL Dec 29, 2016

ogizanagi Dec 29, 2016

ro0NL Dec 29, 2016

ogizanagi Dec 29, 2016

fabpot commented Feb 16, 2017

[Console] Enhance MarkdownDescriptor #20896

[Console] Enhance MarkdownDescriptor #20896

Conversation

ogizanagi commented Dec 13, 2016 • edited Loading

fabpot commented Dec 13, 2016

ogizanagi commented Dec 13, 2016 • edited Loading

ro0NL commented Dec 13, 2016

ogizanagi commented Dec 13, 2016

ro0NL commented Dec 13, 2016

ogizanagi commented Dec 13, 2016 • edited Loading

ro0NL commented Dec 13, 2016 • edited Loading

ogizanagi commented Dec 26, 2016

ro0NL commented Dec 26, 2016 • edited Loading

ro0NL commented Dec 26, 2016 • edited Loading

ogizanagi commented Dec 26, 2016

ogizanagi Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL Dec 29, 2016

Choose a reason for hiding this comment

ogizanagi commented Dec 29, 2016

ro0NL left a comment

Choose a reason for hiding this comment

ro0NL Dec 29, 2016

Choose a reason for hiding this comment

ro0NL Dec 29, 2016

Choose a reason for hiding this comment

ro0NL Dec 29, 2016

Choose a reason for hiding this comment

ogizanagi Dec 29, 2016

Choose a reason for hiding this comment

ro0NL Dec 29, 2016

Choose a reason for hiding this comment

ogizanagi Dec 29, 2016

Choose a reason for hiding this comment

fabpot commented Feb 16, 2017

ogizanagi commented Dec 13, 2016 •

edited

Loading

ogizanagi commented Dec 13, 2016 •

edited

Loading

ogizanagi commented Dec 13, 2016 •

edited

Loading

ro0NL commented Dec 13, 2016 •

edited

Loading

ro0NL commented Dec 26, 2016 •

edited

Loading

ro0NL commented Dec 26, 2016 •

edited

Loading

ogizanagi Dec 26, 2016 •

edited

Loading