Let the DocBlock::ofMethod handle an ~optional~ className #3885

wgevaert · 2019-10-06T17:57:30Z

Fixes issue #3879

codecov · 2019-10-06T18:02:26Z

Codecov Report

Merging #3885 into master will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #3885   +/-   ##
=========================================
  Coverage     83.66%   83.66%           
  Complexity     3853     3853           
=========================================
  Files           151      151           
  Lines         10195    10195           
=========================================
  Hits           8530     8530           
  Misses         1665     1665

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/TestBuilder.php	`100% <ø> (ø)`	`26 <0> (ø)`	⬇️
src/Util/TestDox/NamePrettifier.php	`87.09% <ø> (ø)`	`53 <0> (ø)`	⬇️
src/Util/Test.php	`89.16% <ø> (ø)`	`168 <0> (ø)`	⬇️
src/Util/Annotation/Registry.php	`100% <100%> (ø)`	`8 <3> (ø)`	⬇️
src/Util/Annotation/DocBlock.php	`83.05% <100%> (ø)`	`75 <1> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ee84e...649b496. Read the comment docs.

…t classes

sebastianbergmann · 2019-10-07T06:53:22Z

@Ocramius Can you have a look at this, please? I cannot really put a finger on it, but at first glance this seems more like a workaround than a solution.

Ocramius

Fix seems correct to me, @sebastianbergmann: we are now recording docblocks by hierarchy level in a class hierarchy.

The only problem I see with the patch is that it introduces a change in the test suite, rather than an addition, and that makes it hard to validate it. I suggest reverting test changes, and adding targeted tests instead.

Ocramius · 2019-10-07T06:57:00Z

tests/_files/VariousIterableDataProviderTest.php

 {
-    public static function asArrayProvider()
+    public function asArrayProvider()


Would suggest not changing these as part of thr patch: write independent/new fixtures instead. Changing this means changing tests that might be affected by the bug and/or fix.

I renamed them and wrote similar tests. I hope it is clear, would be happy to hear how it could be done better

I'd generally suggest reverting the old asset changes: they do prevent regressions (like the one that you are fixing - and BTW thanks very much for your efforts!)

I've now kept the old test, but named it testWithVariousIterableStaticDataProviders instead of testWithVariousIterableDataProviders, and then also made tests for when this provider is non-static, of the parent, called from the parent, etc.

Ocramius · 2019-10-07T06:58:45Z

tests/_files/AbstractVariousIterableDataProviderTest.php

+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+abstract class AbstractVariousIterableDataProviderTest


What is being tested with this added fixture? I suggest adding a test for what this is checking.

Ocramius · 2019-10-07T07:00:50Z

src/Util/Annotation/DocBlock.php

@@ -111,7 +114,7 @@ public static function ofMethod(\ReflectionMethod $method): self
            $method->getEndLine(),
            $method->getFileName(),
            $method->getName(),
-            $method->getDeclaringClass()->getName()
+            $className ?? $method->getDeclaringClass()->getName()


If fetching the specific class is the use-case, then I suggest making the new argument mandatory, and naming $className so that it reflects the fact that this is a specific class in the hierarchy.

Absolutely, it's not optional.

Ocramius · 2019-10-07T07:01:36Z

src/Util/Annotation/Registry.php

@@ -62,7 +62,7 @@ public function forClassName(string $class): DocBlock

    /**
     * @throws Exception
-     * @psalm-param class-string $className


Ow, not sure how this made it through 😱

What's with this psalm-param? I do not really understand it, I copied it to more locations, seemed logical to me

This is a restriction on the string type, stating that the string must be a class name. The @psalm-param is an annotation we have to use here, in lack of better type support in phpdocumentor/phpstorm/etc.

All good BTW 👍

Ocramius · 2019-10-07T07:02:10Z

src/Util/Annotation/Registry.php

@@ -80,6 +80,6 @@ public function forMethod(string $class, string $method): DocBlock
            );
        }

-        return $this->methodDocBlocks[$class][$method] = DocBlock::ofMethod($reflection);
+        return $this->methodDocBlocks[$class][$method] = DocBlock::ofMethod($reflection, $class);
    }


$class should probably be named $classInHierarchy or such

gleb-svitelskiy · 2019-10-07T08:19:08Z

src/Util/Annotation/DocBlock.php

+    /**
+     * @psalm-param class-string $className
+     */
+    public static function ofMethod(\ReflectionMethod $method, string $className = null): self


Now there are two ways to create DocBlock object

DocBlock::ofClass( new \ReflectionClass($class) ); DocBlock::ofMethod( new \ReflectionMethod($class, $method) );

Unfortunately, you can not obtain class namespace from method reflection object, only class that declares method.
Maybe it's better to describe only one method to create a DocBlock object e.g.

public static function fromReflection(\ReflectionClass $class, \ReflectionMethod $method = null): self

or make a public constructor

public function __construct(\ReflectionClass $class, \ReflectionMethod $method = null)

@Ocramius what do you think about that?

What about separate named ctors (as separate static methods)?

Also, please keep the ctor private: that's what gave us this flexibility in first place 👍

There is a difference between docblocks of classes and those of methods right? I think the current structure makes that quite clear?

Yeah, all good with that

… abstract classes" This reverts commit 48d3752.

This reverts commit 79c42c9.

wgevaert · 2019-10-07T10:40:52Z

Currently having problems with the following:
Adding the class-string to the classNames for psalm gave problems in Framework/DataProviderTestSuite, in the following snippet:

         [$className, $methodName] = \explode('::', $this->getName());

        return TestUtil::getSize($className, $methodName);

So I made this

         [$className, $methodName] = \explode('::', $this->getName());
 
+        /** @psalm-suppress ArgumentTypeCoercion */
         return TestUtil::getSize($className, $methodName);

Now php-Cs-fixer wants it to look like

         [$className, $methodName] = \explode('::', $this->getName());
 
-        /** @psalm-suppress ArgumentTypeCoercion */
+        /* @psalm-suppress ArgumentTypeCoercion */
         return TestUtil::getSize($className, $methodName);

but then psalm does not recognize the psalm-annotation. What should I do?

Ocramius · 2019-10-07T10:52:18Z

If suppressing won't work, then I suggest updating the baseline file. See psalm --help | grep baseline.

If all is good, we should see that one error added to the file, and if lucky even some error removals in the diff 👍

This reverts commit f57f550.

wgevaert · 2019-10-07T11:08:05Z

Hurray!

sebastianbergmann · 2019-10-07T12:46:58Z

Cherry-picked into 8.4 and merged to master from there. Thank you!

wgevaert · 2019-10-07T13:26:55Z

Did you cherry-pick the whole pull-request or only the first commit? I did make quite some adjustments, thanks to ocramius' feedback, that should certainly be included as well

Edit: Ah, of course you did, I'm sorry for the bump

Let the DocBlock::ofMethod handle an optional className

7170404

Fixes sebastianbergmann#3879

wgevaert added 3 commits October 6, 2019 22:34

Make psalm happy

c39d04e

Make test to test abstract dataproviders and dataproviders in abstrac…

48d3752

…t classes

Correct psalm-param hinting in /Util/Annotation/Registry

d3d5909

Ocramius approved these changes Oct 7, 2019

View reviewed changes

gleb-svitelskiy reviewed Oct 7, 2019

View reviewed changes

wgevaert added 8 commits October 7, 2019 10:44

More psalm class-string typehinting

d5c349c

Revert "Make test to test abstract dataproviders and dataproviders in…

e59d87b

… abstract classes" This reverts commit 48d3752.

Make psalm and csFixer a bit happier

78811f4

Test various different ways of declaring dataProviders

0d41055

Codestyle fixed

e6f1e6d

Last codestyle fix (I hope)

79c42c9

Revert "Last codestyle fix (I hope)"

f57f550

This reverts commit 79c42c9.

add class to DocBlock::ofMethod in TestTest

058d6ab

wgevaert added 2 commits October 7, 2019 13:00

Revert "Revert "Last codestyle fix (I hope)""

89bfa40

This reverts commit f57f550.

Add ArgumentTypeCoercion to psalm baseline

c70d792

Change $class to $classInHierarchy

649b496

Ocramius approved these changes Oct 7, 2019

View reviewed changes

wgevaert changed the title ~~Let the DocBlock::ofMethod handle an optional className~~ Let the DocBlock::ofMethod handle an ~optional~ className Oct 7, 2019

sebastianbergmann closed this Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let the DocBlock::ofMethod handle an ~optional~ className #3885

Let the DocBlock::ofMethod handle an ~optional~ className #3885

wgevaert commented Oct 6, 2019

codecov bot commented Oct 6, 2019 •

edited

Loading

sebastianbergmann commented Oct 7, 2019

Ocramius left a comment

Ocramius Oct 7, 2019

wgevaert Oct 7, 2019

Ocramius Oct 7, 2019

wgevaert Oct 7, 2019

Ocramius Oct 7, 2019

Ocramius Oct 7, 2019

Ocramius Oct 7, 2019

Ocramius Oct 7, 2019

gleb-svitelskiy Oct 7, 2019

Ocramius Oct 7, 2019

wgevaert Oct 7, 2019

Ocramius Oct 7, 2019

Ocramius Oct 7, 2019

gleb-svitelskiy Oct 7, 2019 •

edited

Loading

Ocramius Oct 7, 2019

wgevaert Oct 7, 2019

Ocramius Oct 7, 2019

wgevaert commented Oct 7, 2019

Ocramius commented Oct 7, 2019

wgevaert commented Oct 7, 2019

sebastianbergmann commented Oct 7, 2019

wgevaert commented Oct 7, 2019 •

edited

Loading

Let the DocBlock::ofMethod handle an ~optional~ className #3885

Let the DocBlock::ofMethod handle an ~optional~ className #3885

Conversation

wgevaert commented Oct 6, 2019

codecov bot commented Oct 6, 2019 • edited Loading

Codecov Report

sebastianbergmann commented Oct 7, 2019

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gleb-svitelskiy Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgevaert commented Oct 7, 2019

Ocramius commented Oct 7, 2019

wgevaert commented Oct 7, 2019

sebastianbergmann commented Oct 7, 2019

wgevaert commented Oct 7, 2019 • edited Loading

codecov bot commented Oct 6, 2019 •

edited

Loading

gleb-svitelskiy Oct 7, 2019 •

edited

Loading

wgevaert commented Oct 7, 2019 •

edited

Loading