[Console] Improve code coverage #19328

geoffrey-brier · 2016-07-10T12:12:09Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Hello,

I've added a few tests to improve the console component code coverage (around 3%).

What do you think ?

sstok · 2016-07-10T14:22:08Z

src/Symfony/Component/Console/Application.php

@@ -594,6 +594,8 @@ public function all($namespace = null)
     */
    public static function getAbbreviations($names)
    {
+        @trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0.', __METHOD__), E_USER_DEPRECATED);


Missing deprecation in the phpdoc.

what's the rationale for this deprecation?

@nicolas-grekas While looking for code not covered I noticed that this method wasn't used anywhere so I thought that it should be removed. What's your opinion ?

@geoffrey-brier in any case, this has nothing to do in a PR adding more code coverage

I'd say it's not related to this PR, and if this is the only reason, let's wait for a better reason before deprecating it

All right, I'm gonna remove it then :)

xabbuh · 2016-07-10T21:24:08Z

Test improvements should be done on the 2.7 branch.

bhavin-nakrani · 2016-07-11T16:52:47Z

src/Symfony/Component/Console/Tests/ApplicationTest.php

+    {
+        $application = new CustomDefaultCommandApplication();
+
+        $this->assertNull($application->add(new \DisabledCommand()));


Here $application is always new object so it will null and you assert it should be null. I think it is not helpful testcase.

Should I remove the test then ?

I think change the logic of test case otherwise you can remove this test

Here $application is always new object so it will null and you assert it should be null. I think it is not helpful testcase.

No the assert is on the return value of the call to add(), which passes while it shouldn't.
The PHPDoc states it will always return the command:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Application.php#L379

but it doesn't:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Application.php#L388

I've opened a PR to correct it; #19389

Ok, I'll remove the test case then

stof · 2016-07-11T17:23:23Z

Deprecations? no

This is wrong. Your PR is adding a deprecation (which looks wrong to me)

fabpot · 2016-07-18T15:04:35Z

@geoffrey-brier What's the status of your work here?

geoffrey-brier · 2016-07-19T16:25:14Z

@fabpot I believe I fixed everyone's comment. But as stated by @xabbuh I should reopen the PR against the 2.7 branch. What do you think ?

nicolas-grekas · 2016-07-26T09:06:55Z

👍

phansys · 2016-07-26T11:49:26Z

src/Symfony/Component/Console/Tests/Helper/HelperTest.php

+    public function formatMemoryProvider()
+    {
+        return array(
+            array(0,          '0 B'),


Is this kind of alignment allowed by the CS standards?

array_element_white_space_after_comma [symfony] In array declaration, there MUST be a whitespace after each comma.

(https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/1.11/README.rst)
so no rule about how much whitespace other than something ^_^

I know there are many implicit rules that aren't yet declared @SpacePossum, that's why I'm asking. The reasons I have in mind are the very same that moved us to unalign = and => in the past (#14438), given the code maintainability turns harder when any update to this "structures" is required.

@phansys I formatted the array this way to be consistent with what's already in this file (take a look at the formatTimeProvider function).

Sure @geoffrey-brier, thanks for pointing and putting the sight on this.

nicolas-grekas · 2016-07-28T06:58:13Z

src/Symfony/Component/Console/Command/Command.php

@@ -572,6 +572,8 @@ public function getSynopsis($short = false)
     * Add a command usage example.
     *
     * @param string $usage The usage, it'll be prefixed with the command name
+     *
+     * @return Command


should be @return self (as in other places in the code base where fluent interface are in defined)

it could be even more strict and state @return $this as not only a Command class instance is returned but the instance itself is always returned (for ref. https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#keyword point 13 vs. 15).

nicolas-grekas · 2016-07-28T12:06:18Z

Status: reviewed

nicolas-grekas · 2016-07-28T12:10:45Z

Reminder: for 2.7

geoffrey-brier · 2016-07-28T12:25:23Z

@nicolas-grekas Should I close the PR, rebase it againt the 2.7 branch and reopen a new one ?

fabpot · 2016-07-28T20:21:57Z

src/Symfony/Component/Console/Command/Command.php

@@ -572,6 +572,8 @@ public function getSynopsis($short = false)
     * Add a command usage example.
     *
     * @param string $usage The usage, it'll be prefixed with the command name
+     *
+     * @return $this


we are using @return self everywhere else. Not sure which one is the right one, but I didn't find any occurrence of @return $this in Symfony codebase.

that is because of my suggestion I'm afraid
#19328 (comment)
sorry if misleading the pr!

fabpot · 2016-07-28T20:23:38Z

This one is not mergeable as is. As it contains some small fixes, it should be split. If possible, I would prefer a PR on 2.7 first.

fabpot · 2016-08-15T23:20:24Z

@geoffrey-brier I'm closing this one as it cannot be merge as is. Feel free to reopen smaller and targeted PR. Thanks for your understanding.

carsonbot added Status: Needs Review Console labels Jul 10, 2016

geoffrey-brier force-pushed the improve-console-component-code-coverage branch 3 times, most recently from 689e2e4 to b122d36 Compare July 10, 2016 12:46

sstok reviewed Jul 10, 2016
View reviewed changes

geoffrey-brier force-pushed the improve-console-component-code-coverage branch from b122d36 to 366c69d Compare July 10, 2016 14:52

bhavin-nakrani reviewed Jul 11, 2016
View reviewed changes

geoffrey-brier force-pushed the improve-console-component-code-coverage branch from 366c69d to a9e4ff5 Compare July 19, 2016 16:20

geoffrey-brier force-pushed the improve-console-component-code-coverage branch 3 times, most recently from 0f2b435 to ebb2770 Compare July 26, 2016 08:54

phansys reviewed Jul 26, 2016
View reviewed changes

geoffrey-brier force-pushed the improve-console-component-code-coverage branch from ebb2770 to 38d3b27 Compare July 27, 2016 11:37

geoffrey-brier force-pushed the improve-console-component-code-coverage branch from 38d3b27 to a4a3302 Compare July 27, 2016 11:47

nicolas-grekas reviewed Jul 28, 2016
View reviewed changes

[Console] Improve code coverage

c17be71

geoffrey-brier force-pushed the improve-console-component-code-coverage branch from a4a3302 to c17be71 Compare July 28, 2016 11:52

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 28, 2016

fabpot reviewed Jul 28, 2016
View reviewed changes

fabpot closed this Aug 15, 2016

Uh oh!

[Console] Improve code coverage #19328

[Console] Improve code coverage #19328

Uh oh!

Conversation

geoffrey-brier commented Jul 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

xabbuh commented Jul 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffrey-brier Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

stof commented Jul 11, 2016

Uh oh!

fabpot commented Jul 18, 2016

Uh oh!

geoffrey-brier commented Jul 19, 2016

Uh oh!

nicolas-grekas commented Jul 26, 2016

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!

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!

SpacePossum Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 28, 2016

Uh oh!

nicolas-grekas commented Jul 28, 2016

Uh oh!

geoffrey-brier commented Jul 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 28, 2016

Uh oh!

fabpot commented Aug 15, 2016

Uh oh!

Uh oh!

geoffrey-brier commented Jul 10, 2016 •

edited

Loading

geoffrey-brier Jul 11, 2016 •

edited

Loading

SpacePossum Jul 28, 2016 •

edited

Loading