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

[Console] application/command as text/xml/whatever decoupling#7454

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

Closed

jfsimon wants to merge24 commits intosymfony:masterfromjfsimon:console-descriptor

Closed

[Console] application/command as text/xml/whatever decoupling#7454

jfsimon wants to merge24 commits intosymfony:masterfromjfsimon:console-descriptor

Conversation

Copy link

Contributor

jfsimon commentedMar 22, 2013

This PR removes description generation fromCommand,Application andInputDefinition classes and delegate it to specialized descriptor classes, making it dead simple to add new output formats.

Maybe this could include other commands, likerouter:debug orcontainer:debug (see#5740)?

Add aDescriptorProvider which usesDescriptorInterface objects to describe things.
Addtxt descriptors.
Addxml descriptors.
Addjson descriptors.
Addmd descriptors.
Remove obsolete methods.
Repair tests.

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#6339

stof reviewed

Mar 22, 2013

View reviewed changes

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

Copy link

Member

Choose a reason for hiding this comment

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

it should be@param object if$object can only be an object. And if it can be something else (a scalar or an array), the argument should be renamed)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

changed, thanks

Copy link

Contributor

This PR is massive. It should be broken down into smaller commits that can be reviewed more easily. If possible perhaps even into separate smaller PRs that can be applied atomically without breaking stuff.

jfsimon added10 commits

April 12, 2013 17:46

[Console] added descriptor & refactored description commands

da67c12

[Console] added JSON descriptors

f239b1c

[Console] added markdown descriptors

bebf1eb

[Console] added forgotten headers

43b5e5c

[Console] removed unused methods (tests broken)

acc7414

[Console] fixed tests

774794c

[Console] added XML descriptors

2066a9b

[Console] started text derciptors

47fa194

[Console] finished text descriptors

8b56eb1

[Console] repaired tests

27aa872

Copy link

ContributorAuthor

jfsimon commentedApr 12, 2013

@igorw I just unsquashed my commits for you.

igorw reviewed

Apr 12, 2013

View reviewed changes

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

Copy link

Contributor

igorwApr 12, 2013

Choose a reason for hiding this comment

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

Wouldn't it be a lot simpler if the application text descriptor itself were responsible for rendering its elements? I think it would simplify the design and lead to less implicit dependency on the order of the descriptors.

Copy link

ContributorAuthor

jfsimonApr 12, 2013

Choose a reason for hiding this comment

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

I'm not sure to understand, this class just acts as a "descriptor selector".

Copy link

Contributor

igorw commentedApr 12, 2013

I'm not sure if introducing theDocument is worth the amount of code it introduces. If#6339 can produce the same result with only 342 new LOC, maybe we should aim for something more similar to it.

Copy link

ContributorAuthor

jfsimon commentedApr 12, 2013

@igorw do you think the markdown formatter is overkill?
(I guess the generated documents will change in the future and become more complex).

Copy link

Contributor

igorw commentedApr 12, 2013

Possibly. I guess re-implementing pandoc inside of symfony seems like a bit much.

stof reviewed

Apr 13, 2013

View reviewed changes

src/Symfony/Component/Console/Input/InputDefinition.php Outdated

Copy link

Member

stofApr 13, 2013

Choose a reason for hiding this comment

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

Removing the public method is a BC break. you should provide a BC layer

Copy link

ContributorAuthor

jfsimonApr 13, 2013

Choose a reason for hiding this comment

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

I guessasXml() methods should be present too and must be tagged as@deprecated.

Copy link

Member

stofApr 13, 2013

Choose a reason for hiding this comment

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

right

Copy link

ContributorAuthor

jfsimon commentedApr 13, 2013

@igorw I think your right, I'll simplify the markdown descriptor.

jfsimon added2 commits

April 14, 2013 07:38

[Console] re-introduced asText & asXml methods to avoid BC break

30d8807

[Console] added command definition merge be fore description

9964838

stof reviewed

Apr 14, 2013

View reviewed changes

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

Copy link

Member

stofApr 14, 2013

Choose a reason for hiding this comment

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

Relying on reflection to get the definition looks like a hack

Copy link

Member

stof commentedApr 14, 2013

I don't like the hard coupling of descriptors, where each descriptor creates new instances of other descriptors. I leads to several issues:

this spawned descriptors don't respect the options set in the DescriptorSelector
replacing one of the descriptors in the DescriptorSelector would not be taken into account for nested descriptions.

You should retrieve the needed descriptor from the selector IMO

jfsimon added2 commits

April 14, 2013 20:47

[Console] applied comments from github

49a4612

[Console] added provider injection to descriptors

20c10a5

Copy link

ContributorAuthor

jfsimon commentedApr 14, 2013

@stof @igorw could you please take a look at the last commit, I'm not sure to do it the best way.
Actually, I begin to doubt of the entire feature design.

This was referencedApr 20, 2013

[Console] Add --markdown export to HelpCommand#6339

Closed

[Console] Add --markdown export to HelpCommand#6327

Closed

Copy link

Member

fabpot commentedApr 20, 2013

#5740 should indeed use this framework.@jfsimon Can you update this PR accordingly?

fabpot mentioned this pull request

Apr 20, 2013

[FrameworkBundle] [Console] added format parameter to ContainerDebugCommand#5740

Closed

Copy link

ContributorAuthor

jfsimon commentedApr 20, 2013

@fabpot absolutely. Should I do the same withRouterDebugCommand? Shouldn't this be done in separate PRs?

Copy link

Member

fabpot commentedApr 20, 2013

Let's do that in the same PR (and indeed the router debug command should also be updated).

[Console] simplified descriptors

9c7b358

Copy link

Member

fabpot commentedApr 23, 2013

Can you also removed the*~ files that were committed by mistake?

fabpot reviewed

Apr 23, 2013

View reviewed changes

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

Copy link

Member

fabpotApr 23, 2013

Choose a reason for hiding this comment

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

Can you remove the blank lines between@param lines?

jfsimon added5 commits

April 23, 2013 14:20

[Console] applied advices from github comments

d3ec073

[Console] removed proxy

d70e086

[Console] simplified descriptor interface

ce60fb7

[Console] removed changes

28f082e

[Console] moved commands arguments

cbb8105

fabpot closed this in61c56fc

Apr 23, 2013

jfsimon mentioned this pull request

Apr 30, 2013

[FrameworkBundle] adds routing/container descriptors#7887

Closed

wouterj mentioned this pull request

Jul 7, 2013

Document Descriptors and DescriptorHelpersymfony/symfony-docs#2803

Closed

fabpot added a commit that referenced this pull request

Oct 1, 2013

feature#7887[FrameworkBundle] adds routing/container descriptors (j…

31fb8c7

…fsimon)This PR was squashed before being merged into the master branch (closes#7887).Discussion----------[FrameworkBundle] adds routing/container descriptorsThe goal of this PR is to add descriptors (as in#7454) for routing and container. This will permit add a `--format` option to `router:debug` and `container:debug` commands (with `txt`, `json`, `xml` and `md` formats).Commits-------22f9bc8 [FrameworkBundle] adds routing/container descriptors

ogizanagi mentioned this pull request

Dec 11, 2016

[Console] Improve markdown format#20866

Merged

Labels

None yet

6 participants

Movatterモバイル変換

Uh oh!

[Console] application/command as text/xml/whatever decoupling#7454

[Console] application/command as text/xml/whatever decoupling#7454

Uh oh!

Conversation

jfsimon commentedMar 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorw commentedApr 12, 2013

Uh oh!

jfsimon commentedApr 12, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorw commentedApr 12, 2013

Uh oh!

jfsimon commentedApr 12, 2013

Uh oh!

igorw commentedApr 12, 2013

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!

jfsimon commentedApr 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedApr 14, 2013

Uh oh!

jfsimon commentedApr 14, 2013

Uh oh!

fabpot commentedApr 20, 2013

Uh oh!

jfsimon commentedApr 20, 2013

Uh oh!

fabpot commentedApr 20, 2013

Uh oh!

fabpot commentedApr 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants