Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the missing output variable #4812

Closed
wants to merge 1 commit into from
Closed

Add the missing output variable #4812

wants to merge 1 commit into from

Conversation

harikt
Copy link
Contributor

@harikt harikt commented Jan 13, 2015

Q A
Doc fix? [yes]
New docs? [no]
Applies to [2.5, 2.6]
Fixed tickets [NIL]

For the 2.3 branch there is no table.rst file as it is introduced in 2.5 .

I have checkout from 2.5 and created the PR .

Let me know if I need to send this to master for 2.5 is not LTS .

@@ -25,6 +25,9 @@ To display a table, use :class:`Symfony\\Component\\Console\\Helper\\Table`,
set the headers, set the rows and then render the table::

use Symfony\Component\Console\Helper\Table;
use Symfony\Component\Console\Output\ConsoleOutput;
Copy link
Member

Choose a reason for hiding this comment

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

I would move this before the existing use statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh not sure I understood you. What is the benefit ?

Copy link
Member

Choose a reason for hiding this comment

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

sorry for the confusion, it's right the way it is

@xabbuh
Copy link
Member

xabbuh commented Jan 14, 2015

@harikt Thanks for your pull request. 2.5 was exactly the right target branch as Symfony 2.5 is maintained until the end of this month.

@wouterj
Copy link
Member

wouterj commented Jan 15, 2015

I'm -1 for this change honestly. This code demostrates the table helper. That means you'll always use it in one of the execute/interact/... methods of the Command, which means you'll have access to the $output parameter of those methods.

What's missing instead imo is a simple // ..., which will indicate that something is missing (the class + method). I'm afraid that the current code block would confuse people and makes people instantiating their own $output instead of using the parameter one.

@harikt
Copy link
Contributor Author

harikt commented Jan 15, 2015

@wouterj for you as an experienced symfonian it will be easy and may not find value in it. But if you ask from someone you may get some feedback :) .

@wouterj
Copy link
Member

wouterj commented Jan 16, 2015

@harikt I'm aware that this can be confusing. What about adding the method and class definitions?

  use Symfony\Component\Console\Helper\Table;
+ // ...
+
+ class BookCommand extends Command
+ {
+     public function execute(InputInterface $input, OutputInterface $output)
+     {
- $table = new Table($output);
+         $table = new Table($output);
// ... the rest of the code
+     }
+ }

@harikt
Copy link
Contributor Author

harikt commented Jan 16, 2015

@wouterj when you can run without the command I wonder why you need to show like that. What is the advantage you are seeing ?

@wouterj
Copy link
Member

wouterj commented Jan 23, 2015

@harikt the complete Symfony Console component is designed the way that you use this logic inside Command#execute or Command#interact. This is the official way we document, how would you use this helper?

@weaverryan
Copy link
Member

Hi @harikt!

I actually agree with @wouterj - the "normal" use for this is inside the execute(InputInterface $input, OutputInterface $output) {} of a command, so the $output you're using is passed to you as an argument. However, you do technically bring up a valid usage - you could just use the Table class inside a flat PHP script (not inside a Console component *Command class) and create a ConsoleOutput to render things. Was this your use-case?

If it is, it's not the main usage, so if we show the creation of the ConsoleOutput being done, that will actually be incorrect for most users. I wonder if @wouterj's suggested change above would help - at least for more advanced use-cases, you'd realize that you are not in a Command and may need to create an Output manually.

Thanks!

@harikt
Copy link
Contributor Author

harikt commented Mar 2, 2015

I am ok with the comments #4812 (comment)

@xabbuh
Copy link
Member

xabbuh commented Mar 2, 2015

@harikt Cool, will you be able to make those changes? I think once that's done, the PR is ready to be merged.

@wouterj
Copy link
Member

wouterj commented May 22, 2015

Hi @harikt! There is a doc sprint day tomorrow. I would like to have this PR finished at the end of tomorrow, can you work on it or should we put it on the list so someone else can finish it?

@harikt
Copy link
Contributor Author

harikt commented May 23, 2015

Hey @wouterj ,

You can assign this to someone else. My internet is broken and have very limited access. Else I wished I could. Also got busy with family and things :( .

Enjoy the sprint.

@xabbuh xabbuh added the actionable Clear and specific issues ready for anyone to take them. label May 23, 2015
@harikt
Copy link
Contributor Author

harikt commented Jun 12, 2015

Hey @xabbuh , @wouterj ,

if no one has taken this. I will look into this in the weekend. ( tomorrow / day after tomorrow ) . How about that ?

@harikt
Copy link
Contributor Author

harikt commented Jun 12, 2015

I also noticed one thing . From the https://symfony.com/components there is no link to Validation docs. Why is it not added? Is there any other tickets already ?

@xabbuh
Copy link
Member

xabbuh commented Jun 12, 2015

@harikt This sounds good.

From the https://symfony.com/components there is no link to Validation docs. Why is it not added? Is there any other tickets already ?

They don't exist yet (see #958).

@harikt
Copy link
Contributor Author

harikt commented Jun 13, 2015

@xabbuh I forgot to ask a question. To which branch it should go ?

@harikt
Copy link
Contributor Author

harikt commented Jun 13, 2015

I send a PR to 2.7 as

@xabbuh as already mentioned the time frame

Thanks for your pull request. 2.5 was exactly the right target branch as Symfony 2.5 is maintained until the end of this month.

@wouterj
Copy link
Member

wouterj commented Jun 13, 2015

Thanksfor your PR, @harikt! I'm going to close this one, as it's replaced by your new one.

@wouterj wouterj closed this Jun 13, 2015
xabbuh added a commit that referenced this pull request Jun 28, 2015
…d, so users … (harikt)

This PR was submitted for the 2.7 branch but it was merged into the 2.6 branch instead (closes #5392).

Discussion
----------

Wrap the table creation inside the class extending Command, so users …

…know where the  comes. They can use it as standalone when needed.

This PR is with the changes mentioned in #4812

Commits
-------

00e6d3e Wrap the table creation inside the class extending Command, so users know where the  comes. They can use it as standalone when needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Clear and specific issues ready for anyone to take them. Status: Needs Work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants