[Console] application/command as text/xml/whatever decoupling by jfsimon · Pull Request #7454 · symfony/symfony

jfsimon · 2013-03-22T13:46:03Z

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

Maybe this could include other commands, like router:debug or container:debug (see #5740)?

Add a DescriptorProvider which uses DescriptorInterface objects to describe things.
Add txt descriptors.
Add xml descriptors.
Add json descriptors.
Add md 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 · 2013-03-22T13:53:53Z

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

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)

changed, thanks

igorw · 2013-04-12T15:37:09Z

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 · 2013-04-12T15:56:02Z

@igorw I just unsquashed my commits for you.

igorw · 2013-04-12T15:59:00Z

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

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.

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

igorw · 2013-04-12T16:04:35Z

I'm not sure if introducing the Document 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.

jfsimon · 2013-04-12T16:12:19Z

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

igorw · 2013-04-12T17:43:48Z

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

stof · 2013-04-13T09:25:01Z

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

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

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

jfsimon · 2013-04-13T15:57:20Z

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

stof · 2013-04-14T12:10:37Z

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

Relying on reflection to get the definition looks like a hack

stof · 2013-04-14T12:21:50Z

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 · 2013-04-14T19:41:18Z

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

fabpot · 2013-04-20T13:19:25Z

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

jfsimon · 2013-04-20T13:54:07Z

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

fabpot · 2013-04-20T14:05:51Z

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

fabpot · 2013-04-23T08:52:36Z

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

fabpot · 2013-04-23T08:57:53Z

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

Can you remove the blank lines between @param lines?

…fsimon) This PR was squashed before being merged into the master branch (closes #7887). Discussion ---------- [FrameworkBundle] adds routing/container descriptors The 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

stof reviewed Mar 22, 2013
View reviewed changes

jfsimon added 10 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

igorw reviewed Apr 12, 2013
View reviewed changes

stof reviewed Apr 13, 2013
View reviewed changes

jfsimon added 2 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

jfsimon added 2 commits April 14, 2013 20:47

[Console] applied comments from github

49a4612

[Console] added provider injection to descriptors

20c10a5

This was referenced Apr 20, 2013

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

Closed

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

Closed

fabpot mentioned this pull request Apr 20, 2013

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

Closed

[Console] simplified descriptors

9c7b358

fabpot reviewed Apr 23, 2013
View reviewed changes

jfsimon added 5 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 in 61c56fc 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 DescriptorHelper symfony/symfony-docs#2803

Closed

ogizanagi mentioned this pull request Dec 11, 2016

[Console] Improve markdown format #20866

Merged

Uh oh!

Conversation

jfsimon commented Mar 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorw commented Apr 12, 2013

Uh oh!

jfsimon commented Apr 12, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorw commented Apr 12, 2013

Uh oh!

jfsimon commented Apr 12, 2013

Uh oh!

igorw commented Apr 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 commented Apr 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 14, 2013

Uh oh!

jfsimon commented Apr 14, 2013

Uh oh!

fabpot commented Apr 20, 2013

Uh oh!

jfsimon commented Apr 20, 2013

Uh oh!

fabpot commented Apr 20, 2013

Uh oh!

fabpot commented Apr 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants