8000 Symfony Console Style tweaks by javiereguiluz · Pull Request #15964 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Symfony Console Style tweaks #15964

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Symfony Console Style tweaks #15964

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
162d047
Added a "comment()" helper to show unimportant text
javiereguiluz Sep 28, 2015
8fc9db7
Fix the calculation of the section underline length
javiereguiluz Sep 28, 2015
bbcbddb
Calculate the length of the strings using the Helper methods
javiereguiluz Sep 28, 2015
e4cbfae
Removed recursion
javiereguiluz Sep 28, 2015
9f03c00
Fixed test fixtures
javiereguiluz Sep 28, 2015
176d25f
Removed the method from the interface
javiereguiluz Sep 28, 2015
48efdd9
Fixed tests
javiereguiluz Sep 28, 2015
c65c1d5
Fixed a failing test
javiereguiluz Sep 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Added a "comment()" helper to show unimportant text
  • Loading branch information
@javiereguiluz
javiereguiluz committed Sep 28, 2015
commit 162d04735cbc316e01b79b8a8544da6b733098c5
7 changes: 7 additions & 0 deletions src/Symfony/Component/Console/Style/StyleInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ public function listing(array $elements);
*/
public function text($message);

/**
* Formats secondary text of less importance than text().
*
* @param string|array $message
*/
public function comment($message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a BC break ? I think the StyleInterface is already implemented by some third-party libraries. (But I like the comment method over the text one, it makes much more sense !) Anyway it's a really minor one.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, StyleInterface already was present on 2.7

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've removed the new method from the interface.


/**
* Formats a success result bar.
*
Expand Down
20 changes: 19 additions & 1 deletion src/Symfony/Component/Console/Style/SymfonyStyle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public function text($message)
$this->autoPrependText();

if (!is_array($message)) {
$this->writeln(sprintf(' // %s', $message));
$this->writeln(sprintf(' %s', $message));

return;
}
Expand All @@ -160,6 +160,24 @@ public function text($message)
}
}

/**
* {@inheritdoc}
*/
public function comment($message)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not defined in the parent class. So inheritdoc makes no sense. Also I guess this method should be part of StyleInterface. But I don't really see the point of this interface either.

Copy link
Member

Choose a reason for hiding this comment

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

I'm very glad that I'm not the only one to wonder about why one insists on having interfaces for everything. Here again, I'm 👍 for removing this interface.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, the interface is useless as we always create concrete instances instead of injecting them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can image a user-case: Having a centralized place where the style is created. And all commands would just work with StyleInterface. Thus one can easily replace the style evewhere. But that is not how commands currently work.

Copy link
Member

Choose a reason for hiding this comment

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

I am +1 for removing the interface as well. There was a reason for it that was discussed during the initial PR but I can't find it. Laravel is using this feature but they extend SymfonyStyle.

{
$this->autoPrependText();

if (!is_array($message)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be simplified to something like:

$this->autoPrependText();
$messages = is_array($messages) ? array_values($messages) : array($messages);
foreach ($messages as $message) {
     $this->writeln(sprintf(' // %s', $message));
}

Unless you really need the recursion to do something like:

$sfStyle->comment([
    array('lorem', array('ipsum', 'dolor')),
    'Sit',
    'amet',
])

Which would be kind of confusing and should be reflected to other similar methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this proposal. If @kbond (the original author of this code) doesn't say anything against it, I'll do these changes. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ! Actually I didn't see it was the same implementation as text... It would be a minor BC break again if someone rely on this particular behavior.
Would string[]|string be more accurate in the docblock ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the recursion is necessary. Simplify away :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes made. Thanks.

$this->writeln(sprintf(' // %s', $message));

return;
}

foreach ($message as $element) {
$this->comment($element);
}
}

/**
* {@inheritdoc}
*/
Expand Down
0