[Console] Review Application docblocks #20813

ogizanagi · 2016-12-07T12:10:22Z

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

I know there must be a lot of other places in the core where there is some repeated or useless informations in docblocks, but everytime I dig into the Application class, I see this, and I don't want to repeat things for consistency when adding new methods 😅 (for instance in #20808, the setCatchThrowables / areThrowablesCaught methods do not need a docblock description IMHO).

~~This PR adapts docblocks where:~~

~~A docblock description is not required, as everything can be expressed in the @return / @param argument (the case mentioned above)~~
~~Information is redundant between description and tags, and the context does not have to be reminded again:~~

    /**
     * Adds an array of command objects.
     *
     * If a Command is not enabled it will not be added.
     *
-     * @param Command[] $commands An array of commands
+     * @param Command[] $commands
     */
    public function addCommands(array $commands)
    {
        foreach ($commands as $command) {
            $this->add($command);
        }
    }

fabpot · 2016-12-07T12:49:07Z

I'm 👎 on these changes. In theory, I agree with them, but as it makes merging more complex, and makes diffs between version non-readable, I think leaving them as is is the best thing to do. For new phpdocs, we should stick with the new "rules".

ogizanagi · 2016-12-07T14:15:49Z

Of course, I understand and expected your comment 😅 .
If those changes are actually rejected, maybe we can at least keep the ones about adding the @throws Throwable and associated descriptions for PHP 7?

fabpot · 2016-12-07T14:19:52Z

Fixes on current docblocks are indeed acceptable :)

fabpot · 2016-12-07T14:20:12Z

src/Symfony/Component/Console/Application.php

@@ -103,7 +101,8 @@ public function setDispatcher(EventDispatcherInterface $dispatcher)
     *
     * @return int 0 if everything went fine, or an error code
     *
-     * @throws \Exception When doRun returns Exception
+     * @throws \Exception When doRun threw an exception and catchExceptions is false
+     * @throws \Throwable When doRun threw a throwable


Should be merged with the previous line \Exception|\Throwable

Can be, but the aim was to point the fact all throwables are not caught by the application (yet), and thus not handled nicely, whereas as this can be configured for exceptions by setting catchExceptions to true.

#20808 replicates this behavior for throwables by adding a new catchThrowables property.

this should be removed entirely actually. It does not make any sense to document that it throws an exception when the inner code threw an exception. PHP does not require each class to document exceptions thrown in called codes (unlike Java)

nicolas-grekas · 2016-12-08T14:31:55Z

This looks wrong to me. We don't document "rethrowing"

ogizanagi · 2016-12-08T14:57:13Z

this should be removed entirely actually. It does not make any sense to document that it throws an exception when the inner code threw an exception. PHP does not require each class to document exceptions thrown in called codes (unlike Java)

This looks wrong to me. We don't document "rethrowing"

My opinion on this is that it matters only when it has value for the end user.
Application is an entrypoint. It may be useful to know if it handles gracefully or not exceptions for you, especially if it is conditional (the catchExceptions flag).

ogizanagi · 2016-12-13T00:23:30Z

So, I'm not trying to be stubborn here, but simply explaining my thoughts on this in order to get yours 😃

What should be done here now?

Merge as is: Adds the Throwable type and document under what conditions it's thrown, because it has a value for the end user, as Application is an entrypoint.
Remove @throw tags from Application::run(), because we don't document rethrowing at all
Something else?
Close this PR

fabpot · 2016-12-13T08:33:56Z

I would vote for 2

ogizanagi · 2016-12-13T15:16:17Z

Done. Thanks for the answers. :)

stof · 2016-12-13T15:17:34Z

src/Symfony/Component/Console/Application.php

@@ -832,7 +830,7 @@ protected function configureIO(InputInterface $input, OutputInterface $output)
     *
     * @return int 0 if everything went fine, or an error code
     *
-     * @throws \Exception when the command being run threw an exception
+     * @throws \Throwable|\Exception when the command being run threw a throwable


should be removed too here, as this is also documenting a rethrowing

Indeed. 👍

fabpot · 2016-12-13T15:28:08Z

Thank you @ogizanagi.

@param

This PR was squashed before being merged into the 2.7 branch (closes #20813). Discussion ---------- [Console] Review Application docblocks | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A ~I know there must be a lot of other places in the core where there is some repeated or useless informations in docblocks, but everytime I dig into the `Application` class, I see this, and I don't want to repeat things for consistency when adding new methods 😅 (for instance in #20808, the `setCatchThrowables / areThrowablesCaught ` methods do not need a docblock description IMHO).~ ~This PR adapts docblocks where:~ - ~A docblock description is not required, as everything can be expressed in the `@return / @param` argument (the case mentioned above)~ - ~Information is redundant between description and tags, and the context does not have to be reminded again:~ ```diff /** * Adds an array of command objects. * * If a Command is not enabled it will not be added. * - * @param Command[] $commands An array of commands + * @param Command[] $commands */ public function addCommands(array $commands) { foreach ($commands as $command) { $this->add($command); } } ``` Commits ------- d8c18cc [Console] Review Application docblocks

[Console] Review Application docblocks

3ab4262

carsonbot added Status: Needs Review Console labels Dec 7, 2016

nicolas-grekas modified the milestone: 2.7 Dec 7, 2016

fabpot reviewed Dec 7, 2016

View reviewed changes

stof reviewed Dec 13, 2016

View reviewed changes

[Console] Review Application docblocks

a80c5f7

fabpot closed this Dec 13, 2016

ogizanagi deleted the console/docblocks branch December 13, 2016 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Review Application docblocks #20813

[Console] Review Application docblocks #20813

ogizanagi commented Dec 7, 2016 •

edited

Loading

fabpot commented Dec 7, 2016

ogizanagi commented Dec 7, 2016

fabpot commented Dec 7, 2016

fabpot Dec 7, 2016

ogizanagi Dec 7, 2016 •

edited

Loading

stof Dec 8, 2016

nicolas-grekas commented Dec 8, 2016

ogizanagi commented Dec 8, 2016 •

edited

Loading

ogizanagi commented Dec 13, 2016 •

edited

Loading

fabpot commented Dec 13, 2016

ogizanagi commented Dec 13, 2016

stof Dec 13, 2016

ogizanagi Dec 13, 2016

fabpot commented Dec 13, 2016

[Console] Review Application docblocks #20813

[Console] Review Application docblocks #20813

Conversation

ogizanagi commented Dec 7, 2016 • edited Loading

fabpot commented Dec 7, 2016

ogizanagi commented Dec 7, 2016

fabpot commented Dec 7, 2016

fabpot Dec 7, 2016

Choose a reason for hiding this comment

ogizanagi Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

stof Dec 8, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Dec 8, 2016

ogizanagi commented Dec 8, 2016 • edited Loading

ogizanagi commented Dec 13, 2016 • edited Loading

fabpot commented Dec 13, 2016

ogizanagi commented Dec 13, 2016

stof Dec 13, 2016

Choose a reason for hiding this comment

ogizanagi Dec 13, 2016

Choose a reason for hiding this comment

fabpot commented Dec 13, 2016

ogizanagi commented Dec 7, 2016 •

edited

Loading

ogizanagi Dec 7, 2016 •

edited

Loading

ogizanagi commented Dec 8, 2016 •

edited

Loading

ogizanagi commented Dec 13, 2016 •

edited

Loading