NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/console/show_hidden_cmd_json_xml

Jan 4, 2017

Merged

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

fabpot merged 1 commit intosymfony:masterfromogizanagi:feature/console/show_hidden_cmd_json_xml

Jan 4, 2017

Conversation

Copy link

Contributor

ogizanagi commentedDec 28, 2016

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

[Console] Show hidden commands in json & xml descriptors

b6cf240

carsonbot added Status: Needs Review Console Feature labels

Dec 28, 2016

Copy link

Contributor

ro0NL commentedDec 28, 2016

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

Copy link

ContributorAuthor

ogizanagi commentedDec 28, 2016

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 totrue/false (InputOption::VALUE_OPTIONAL with default acting as auto).

Anyway, let's first approve this one before.

Copy link

Contributor

ro0NL commentedDec 28, 2016•
edited
Loading

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.

Copy link

ContributorAuthor

ogizanagi commentedDec 28, 2016•
edited
Loading

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.

Copy link

Member

fabpot commentedDec 28, 2016

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.

Copy link

Contributor

ro0NL commentedDec 28, 2016

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 reviewed

Dec 28, 2016

View reviewed changes

Copy link

Contributor

ro0NL left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Status: Reviewed

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

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

Copy link

Contributor

ro0NLDec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Getting aApplicationDescription from options looks tedious..

Copy link

ContributorAuthor

ogizanagiDec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What do you mean? 😕

Copy link

Contributor

ro0NLDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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


		####`--show-hidden`

		To show hidden commands

Copy link

Contributor

ro0NLDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

MaybeTo 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.

Copy link

ContributorAuthor

ogizanagiDec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why not.include indeed looks better to me :)

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

		* @param string\|null $namespace
		* @param bool $showHidden
		*/
		publicfunction__construct(Application$application,$namespace =null)

Copy link

Contributor

ro0NLDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		newInputArgument('namespace', InputArgument::OPTIONAL,'The namespace name'),
		newInputOption('raw',null, InputOption::VALUE_NONE,'To output raw command list'),
		newInputOption('format',null, InputOption::VALUE_REQUIRED,'The output format (txt, xml, json, or md)','txt'),
		newInputOption('show-hidden',null, InputOption::VALUE_NONE,'To show hidden commands'),

Copy link

Contributor

ro0NLDec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

ogizanagi commentedDec 28, 2016

There is one more thing to consider regarding the--show-hidden option: It feels strange for bothxml andjson formats to get thehidden 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.

Copy link

Contributor

ro0NL commentedDec 28, 2016

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

ro0NL reviewed

Dec 28, 2016

View reviewed changes

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

		*/
		protectedfunctiondescribeApplication(Application$application,array$options =array())
		{
		$describedNamespace =isset($options['namespace']) ?$options['namespace'] :null;

Copy link

Contributor

ro0NLDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The suggestion ofgetApplicationDescription 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.

Copy link

Member

fabpot commentedDec 30, 2016

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

Copy link

ContributorAuthor

ogizanagi commentedDec 30, 2016

--show-hidden option removed :)

ro0NL reviewed

Dec 30, 2016

View reviewed changes

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

		{
		$describedNamespace =isset($options['namespace']) ?$options['namespace'] :null;
		$description =newApplicationDescription($application,$describedNamespace);
		$description =newApplicationDescription($application,$describedNamespace,true);

Copy link

Contributor

ro0NLDec 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

nicolas-grekas added this to the3.x milestone

Jan 2, 2017

Copy link

Member

fabpot commentedJan 4, 2017

Thank you@ogizanagi.

fabpot merged commitb6cf240 intosymfony:master

Jan 4, 2017

fabpot added a commit that referenced this pull request

Jan 4, 2017

feature#21075[Console] Show hidden commands in json & xml descripto…

df38318

…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/ACommits-------b6cf240 [Console] Show hidden commands in json & xml descriptors

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 descriptorssymfony/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

Labels

Console Feature Status: Needs Review

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

ogizanagi commentedDec 28, 2016

Uh oh!

ro0NL commentedDec 28, 2016

Uh oh!

ogizanagi commentedDec 28, 2016

Uh oh!

ro0NL commentedDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ogizanagi commentedDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

fabpot commentedDec 28, 2016

Uh oh!

ro0NL commentedDec 28, 2016

Uh oh!

ro0NL left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NLDec 28, 2016

Choose a reason for hiding this comment

Uh oh!

ogizanagiDec 28, 2016

Choose a reason for hiding this comment

Uh oh!

ro0NLDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NLDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagiDec 28, 2016

Choose a reason for hiding this comment

Uh oh!

ro0NLDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NLDec 28, 2016

Choose a reason for hiding this comment

Uh oh!

ogizanagi commentedDec 28, 2016

Uh oh!

ro0NL commentedDec 28, 2016

Uh oh!

ro0NLDec 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedDec 30, 2016

Uh oh!

ogizanagi commentedDec 30, 2016

Uh oh!

ro0NLDec 30, 2016

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJan 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ro0NL commentedDec 28, 2016•
edited
Loading

ogizanagi commentedDec 28, 2016•
edited
Loading

ro0NL left a comment•
edited
Loading

ro0NLDec 28, 2016•
edited
Loading

ro0NLDec 28, 2016•
edited
Loading

ro0NLDec 28, 2016•
edited
Loading

ro0NLDec 28, 2016•
edited
Loading