8000 [Console][DescriptorHelper] plugs the docu for 2.3, a second PR follows after this is merged by cordoval · Pull Request #3430 · symfony/symfony-docs · GitHub
[go: up one dir, main page]

Skip to content

[Console][DescriptorHelper] plugs the docu for 2.3, a second PR follows after this is merged#3430

Closed
cordoval wants to merge 1 commit intosymfony:2.3from
cordoval:2803-document-descriptors-and-descriptorhelper
Closed

[Console][DescriptorHelper] plugs the docu for 2.3, a second PR follows after this is merged#3430
cordoval wants to merge 1 commit intosymfony:2.3from
cordoval:2803-document-descriptors-and-descriptorhelper

Conversation

@cordoval
Copy link
Copy Markdown
Contributor
@cordoval cordoval commented Jan 6, 2014
Q A
Doc fix? no
New docs? yes
Applies to 2.3+
Fixed tickets #2803
License CC-ASA 3.0 Unported

Sent using Gush

@cordoval cordoval restored the 2803-document-descriptors-and-descriptorhelper branch January 6, 2014 15:49
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be indented by 4 spaces

@cordoval
Copy link
Copy Markdown
Contributor Author

@weaverryan are these adaptations enough or we still think more is needed. I am ready to comply, just that i am posing an opinion as well, but please don't hesitate to tell me if i should override.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do you wrap the line here?

@cordoval
Copy link
Copy Markdown
Contributor Author

@wouterj not sure why you removed your comment, but you did. I guess you realized that HelpCommand is not the focus here. That helper is a mechanism bus to plug and use the descriptors in other commands where they are actually used.

@wouterj
Copy link
Copy Markdown
Member
wouterj commented Jan 11, 2014

@cordoval I did not. It's still there, hidden in the oudated diff: #3430 (comment)

And we are talking about the help command here. The default Descriptor can only render help of a command, input definition, etc. That'll not be helpfull in other commands than the help command, does it?

@cordoval
Copy link
Copy Markdown
Contributor Author

@wouterj the only application for descriptors is not the help command. That is why you add only custom descriptors to the helper. Of course it will.

@wouterj
Copy link
Copy Markdown
Member
wouterj commented Jan 11, 2014

@cordoval but the point is that the example you gave is only talking about the default command descriptor. You should not use the available implementation, only the DescriptorHelper class and DescriptorInterface interface are helpfull. Your example talks about a NoColorDescriptor which extends Descriptor, which means you are talking about describing a command/input definition/... which is not helpfull in other commands.

It looks like you completely missed the second part of both Ryan's and my comments.

@cordoval
Copy link
Copy Markdown
Contributor Author

no, my paragraphs were not talking about only the default command descriptor. People can be told what they can do, they can extend or implement, is their choice. Even when extending it is more for reuse the good part, if you want to use other parts you extend them or finally implement. so no, even with the input_definition stuff, it can be reused because is common, nobody says that you will keep all the defaults. I think when descriptors was created was keeping in mind this.

I don't think i have missed those parts. I just disagree slightly.

@wouterj
Copy link
Copy Markdown
Member
wouterj commented Jan 11, 2014

We can write the NoColorDescriptor implementing the methods::

NoColorDescriptor::describeInputArgument
NoColorDescriptor::describeInputOption
NoColorDescriptor::describeInputDefinition
NoColorDescriptor::describeCommand
NoColorDescriptor::describeApplication

I think this is pretty clear that it's overriding and creating a new Descriptor class for commands and not for other things. You should explain creating descriptors for other things, like a Users object. Then you only need the DescriptorInterface which requires a describe method only. You would generate something like:

<?php

use Symfony\Component\Console\Descriptor\DescriptorInterface;

use Acme\User;
use Acme\UserGroup;

abstract class UserDescriptor implements DescriptorInterface
{
    protected $output;

    public function describe(OutputInterface $output, $subject, array $options = array())
    {
        $this->output = $output;
        if ($subject instanceof User) {
            $this->describeUser($subject);
        } elseif ($subject instanceof UserGroup) {
            $this->describeUserGroup($subject);
        }
    }

    abstract protected function describeUser(User $subject);
    abstract protected function describeUserGroup(UserGroup $subject);
}

use Symfony\Component\Console\Helper\TableHelper;

class TextUserDescriptor extends UserDescriptor
{
    protected function describeUser(User $subject)
    {
        $lines = array();
        $lines[] = 'Name:  '.$subject->getName();
        $lines[] = 'Age:   '.$subject->getAge();
        $lines[] = 'Group: '.$subject->getGroup()->getName();

        $this->output->writeln($lines);
    }

    protected function describeUserGroup(UserGroup $subject)
    {
        $table = new TableHelper();
        $table->setLayout(TableHelper::LAYOUT_COMPACT);
        $table->setHeaders(array('Name', 'Age'));

        foreach ($subject->getUsers() as $user) {
            $table->addRow(array($user->getName(), $user->getAge()));
        }

        $this->output->writeln(array(
            'Group name: '.$subject->getName(),
            '',
        ));
        $table->render($this->output);
    }
}

In that case, this article will be helpfull. Currently, I don't see what this article tries to explain to me.

@cordoval
Copy link
Copy Markdown
Contributor Author

yes this is a good point @wouterj, i fully agree with showing this example and then that should be enough to get it across. I will work on this if we are agreeing on this. Now it is clear what needs to be done. Thanks for the bearing with me 👶

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bash commands should be prefixed with $

@cordoval
Copy link
Copy Markdown
Contributor Author

👶

~ gsh p:squash 3430             
8000
                     Luiss-MacBook-Pro-3 [21:42:45]
OUT > Fetching origin
OUT > Fetching cordoval
ERR > Already on '2803-document-descriptors-and-descriptorhelper'
OUT > Your branch is up-to-date with 'cordoval/2803-document-descriptors-and-descriptorhelper'.
OUT > [2803-document-descriptors-and-descriptorhelper 121e5bf] 2803-document-descriptors-and-descriptorhelper
 4 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 components/console/helpers/descriptorhelper.rst
ERR > To git@github.com:cordoval/symfony-docs.git
ERR >  + 94ac9cf...121e5bf 2803-document-descriptors-and-descriptorhelper -> 2803-document-descriptors-and-descriptorhelper (forced update)
OUT > Branch 2803-document-descriptors-and-descriptorhelper set up to track remote branch 2803-document-descriptors-and-descriptorhelper from cordoval.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use a list here, no code block

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

show me please an example that I may learn this, thanks man

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

0