[Console] Show hidden commands in json & xml descriptors #21075

ogizanagi · 2016-12-28T12:28:44Z

Q	A
Branch?	master
Bug fix?	no (may be considered, but as hidden commands did not exist before 3.2, looks more like a behavior change, hence a feature)
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20866 (comment)
License	MIT
Doc PR	N/A

ro0NL · 2016-12-28T12:41:09Z

Nice 👍 what about bin/console list --show-hidden? The idea of hidden commands was to exclude them by default (for not having a contextual relevance).

ogizanagi · 2016-12-28T14:37:00Z

Looks like an idea to me :)

But I'd like to keep meaningful defaults based on the format unless the --show-hidden flag is provided/explicitly set to true/false (InputOption::VALUE_OPTIONAL with default acting as auto).

Anyway, let's first approve this one before.

ro0NL · 2016-12-28T15:08:46Z

meaningful defaults based on the format

Technically they dont know about each other.. they should not make assumptions regarding context imo. The only context we do have is "hidden", probably meaning; dont bother the user with it.

So 👍 for defaulting to false, where defaulting is needed. But i understand you actually chose to differ between XML/JSON and markdown here. Always providing the information technically in that case.

I think we should do format based decision making at command level. More close to real user decision making. Although it could still be experienced counter-intuitive. No real opinion here :)

Anyway, let's first approve this one before.

Im not sure you wanna do 2 PR's? But imo. it can be 1, doing the feature as a whole. Right now master is good (imo.); consistently hidden by default. Not sure we want this intermediate step, or as final solution for that matter, again imo.

For me 👍 adding --show-hidden to bypass the hardcoded default.

ogizanagi · 2016-12-28T16:45:41Z

Im not sure you wanna do 2 PR's? But imo. it can be 1, doing the feature as a whole.

Just waiting for some more feedbacks on this and the option suggestion.

But ok, done.

fabpot · 2016-12-28T16:56:39Z

I think I'm 👎 on the --show-hidden option. That would defeat the whole purpose of the hidden feature as accepted when it was introduced.

ro0NL · 2016-12-28T17:09:39Z

Sounds reasonable. But it really depends what "hidden" means. Is it only from a end-user perspective? GUI-wise?

Then the initial (by now ;-)) approach makes sense. Including for technical formats, excluding for human formats.

ro0NL

LGTM 👍 basically only having the console option yes/no is questionable.

Status: Reviewed

ro0NL · 2016-12-28T18:11:20Z

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

-        $description = new ApplicationDescription($application, $namespace);
+        $namespace = isset($options['namespace']) ? $options['namespace'] : null;
+        $showHidden = isset($options['show_hidden']) ? $options['show_hidden'] : false;
+        $description = new ApplicationDescription($application, $namespace, $showHidden);


Getting a ApplicationDescription from options looks tedious..

What do you mean? 😕

Descriptor::getApplicationDescription($options) could be worth it by now (2 options * 4 descriptors)

ro0NL · 2016-12-28T18:14:45Z

src/Symfony/Component/Console/Tests/Fixtures/application_2.md

@@ -178,6 +178,15 @@ The output format (txt, xml, json, or md)
 * Is multiple: no
 * Default: `'txt'`

+#### `--show-hidden`
+
+To show hidden commands


Maybe To include hidden commands, it kinda implies we lose the other ones. Anyway this made me doubt between --show-hidden and --with-hidden. Im fine either way, but perhaps be explicit in its description.

Why not. include indeed looks better to me :)

ro0NL · 2016-12-28T18:15:37Z

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

     */
-    public function __construct(Application $application, $namespace = null)
+    public function __construct(Application $application, $namespace = null, $showHidden = false)


This feature is good for sure 👍 same for updated outputs.

ro0NL · 2016-12-28T18:16:40Z

src/Symfony/Component/Console/Command/ListCommand.php

@@ -85,6 +86,7 @@ private function createDefinition()
            new InputArgument('namespace', InputArgument::OPTIONAL, 'The namespace name'),
            new InputOption('raw', null, InputOption::VALUE_NONE, 'To output raw command list'),
            new InputOption('format', null, InputOption::VALUE_REQUIRED, 'The output format (txt, xml, json, or md)', 'txt'),
+            new InputOption('show-hidden', null, InputOption::VALUE_NONE, 'To show hidden commands'),


No opinion (as we can also use application descriptors in code, so as calling commands).

ogizanagi · 2016-12-28T18:32:39Z

There is one more thing to consider regarding the --show-hidden option: It feels strange for both xml and json formats to get the hidden information always added as an attribute. If it doesn't include hidden commands, this is pretty useless.

I'll wait some more opinions and remove the option if it doesn't seem appropriated.

ro0NL · 2016-12-28T18:42:14Z

Not sure, we wanna micro optimize that? The definition is clear, allowing everybody to rely on it consistently.

ro0NL · 2016-12-28T21:35:39Z

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

@@ -64,7 +64,7 @@ protected function describeCommand(Command $command, array $options = array())
    protected function describeApplication(Application $application, array $options = array())
    {
        $describedNamespace = isset($options['namespace']) ? $options['namespace'] : null;


The suggestion of getApplicationDescription was to move (global) option management to a single place. Keeping this doesnt solve that, gaining only 1 option * 4 descriptors.

Imo. we should rely on some $description->getNamespace() as of below, otherwise maybe revert.

fabpot · 2016-12-30T06:29:53Z

"Then the initial (by now ;-)) approach makes sense. Including for technical formats, excluding for human formats." -> like that.

ogizanagi · 2016-12-30T10:22:59Z

--show-hidden option removed :)

ro0NL · 2016-12-30T10:26:15Z

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

@@ -64,7 +64,7 @@ protected function describeCommand(Command $command, array $options = array())
    protected function describeApplication(Application $application, array $options = array())
    {
        $describedNamespace = isset($options['namespace']) ? $options['namespace'] : null;
-        $description = new ApplicationDescription($application, $describedNamespace);
+        $description = new ApplicationDescription($application, $describedNamespace, true);


Imo. the descriptor option was nice :) anyway it's internal code.. we're good 👍

fabpot · 2017-01-04T14:29:26Z

Thank you @ogizanagi.

…rs (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [Console] Show hidden commands in json & xml descriptors | Q | A | ------------- | --- | Branch? | master | Bug fix? | no (may be considered, but as hidden commands did not exist before 3.2, looks more like a behavior change, hence a feature) | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20866 (comment) | License | MIT | Doc PR | N/A Commits ------- b6cf240 [Console] Show hidden commands in json & xml descriptors

[Console] Show hidden commands in json & xml descriptors

b6cf240

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

ro0NL reviewed Dec 28, 2016

View reviewed changes

ro0NL reviewed Dec 30, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Jan 2, 2017

fabpot merged commit b6cf240 into symfony:master Jan 4, 2017

ogizanagi deleted the feature/console/show_hidden_cmd_json_xml branch January 4, 2017 14:30

javiereguiluz mentioned this pull request Jan 8, 2017

Hidden console commands are not hidden in JSON and XML descriptors symfony/symfony-docs#7335

Closed

ogizanagi mentioned this pull request Jan 9, 2017

[Console] Add a way to check if an argument/option was provided #21210

Closed

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

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Show hidden commands in json & xml descriptors #21075

[Console] Show hidden commands in json & xml descriptors #21075

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016 •

edited

Loading

ogizanagi commented Dec 28, 2016 •

edited

Loading

fabpot commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ro0NL left a comment •

edited

Loading

ro0NL Dec 28, 2016

ogizanagi Dec 28, 2016

ro0NL Dec 28, 2016 •

edited

Loading

ro0NL Dec 28, 2016 •

edited

Loading

ogizanagi Dec 28, 2016

ro0NL Dec 28, 2016 •

edited

Loading

ro0NL Dec 28, 2016

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ro0NL Dec 28, 2016 •

edited

Loading

fabpot commented Dec 30, 2016

ogizanagi commented Dec 30, 2016

ro0NL Dec 30, 2016

fabpot commented Jan 4, 2017

[Console] Show hidden commands in json & xml descriptors #21075

[Console] Show hidden commands in json & xml descriptors #21075

Conversation

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016 • edited Loading

ogizanagi commented Dec 28, 2016 • edited Loading

fabpot commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ro0NL left a comment • edited Loading

Choose a reason for hiding this comment

ro0NL Dec 28, 2016

Choose a reason for hiding this comment

ogizanagi Dec 28, 2016

Choose a reason for hiding this comment

ro0NL Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

ogizanagi Dec 28, 2016

Choose a reason for hiding this comment

ro0NL Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL Dec 28, 2016

Choose a reason for hiding this comment

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016

ro0NL Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

fabpot commented Dec 30, 2016

ogizanagi commented Dec 30, 2016

ro0NL Dec 30, 2016

Choose a reason for hiding this comment

fabpot commented Jan 4, 2017

ro0NL commented Dec 28, 2016 •

edited

Loading

ogizanagi commented Dec 28, 2016 •

edited

Loading

ro0NL left a comment •

edited

Loading

ro0NL Dec 28, 2016 •

edited

Loading

ro0NL Dec 28, 2016 •

edited

Loading

ro0NL Dec 28, 2016 •

edited

Loading

ro0NL Dec 28, 2016 •

edited

Loading