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.

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

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

Closed
wants to merge 15 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7170404
Let the DocBlock::ofMethod handle an optional className
wgevaert Oct 6, 2019
c39d04e
Make psalm happy
wgevaert Oct 6, 2019
48d3752
Make test to test abstract dataproviders and dataproviders in abstrac…
wgevaert Oct 6, 2019
d3d5909
Correct psalm-param hinting in /Util/Annotation/Registry
wgevaert Oct 6, 2019
d5c349c
More psalm class-string typehinting
wgevaert Oct 7, 2019
e59d87b
Revert "Make test to test abstract dataproviders and dataproviders in…
wgevaert Oct 7, 2019
78811f4
Make psalm and csFixer a bit happier
wgevaert Oct 7, 2019
0d41055
Test various different ways of declaring dataProviders
wgevaert Oct 7, 2019
e6f1e6d
Codestyle fixed
wgevaert Oct 7, 2019
79c42c9
Last codestyle fix (I hope)
wgevaert Oct 7, 2019
f57f550
Revert "Last codestyle fix (I hope)"
wgevaert Oct 7, 2019
058d6ab
add class to DocBlock::ofMethod in TestTest
wgevaert Oct 7, 2019
89bfa40
Revert "Revert "Last codestyle fix (I hope)""
wgevaert Oct 7, 2019
c70d792
Add ArgumentTypeCoercion to psalm baseline
wgevaert Oct 7, 2019
649b496
Change $class to $classInHierarchy
wgevaert Oct 7, 2019
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
7 changes: 5 additions & 2 deletions src/Util/Annotation/DocBlock.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ public static function ofClass(\ReflectionClass $class): self
);
}

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

@gleb-svitelskiy gleb-svitelskiy Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, all good with that

{
return new self(
(string) $method->getDocComment(),
Expand All @@ -111,7 +114,7 @@ public static function ofMethod(\ReflectionMethod $method): self
$method->getEndLine(),
$method->getFileName(),
$method->getName(),
$method->getDeclaringClass()->getName()
$className ?? $method->getDeclaringClass()->getName()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely, it's not optional.

);
}

Expand Down
4 changes: 2 additions & 2 deletions src/Util/Annotation/Registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function forClassName(string $class): DocBlock

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

Choose a reason for hiding this comment

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

Ow, not sure how this made it through 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

* @psalm-param class-string $class
*/
public function forMethod(string $class, string $method): DocBlock
{
Expand All @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

$class should probably be named $classInHierarchy or such

}
56 changes: 56 additions & 0 deletions tests/_files/AbstractVariousIterableDataProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php declare(strict_types=1);
/*
* This file is part of PHPUnit.
*
* (c) Sebastian Bergmann <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
abstract class AbstractVariousIterableDataProviderTest
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved

{
abstract public function asArrayProvider();

abstract public function asIteratorProvider();

abstract public function asTraversableProvider();

public function asArrayProviderInParent()
{
return [
['J'],
['K'],
['L'],
];
}

public function asIteratorProviderInParent()
{
yield ['M'];

yield ['N'];

yield ['O'];
}

public function asTraversableProviderInParent()
{
return new WrapperIteratorAggregate([
['P'],
['Q'],
['R'],
]);
}

/**
* @dataProvider asArrayProvider
* @dataProvider asIteratorProvider
* @dataProvider asTraversableProvider
* @dataProvider asArrayProviderInParent
* @dataProvider asIteratorProviderInParent
* @dataProvider asTraversableProviderInParent
*/
public function testAbstract(): void
{
}
}
8 changes: 4 additions & 4 deletions tests/_files/VariousIterableDataProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
class VariousIterableDataProviderTest
class VariousIterableDataProviderTest extends AbstractVariousIterableDataProviderTest
{
public static function asArrayProvider()
public function asArrayProvider()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

{
return [
['A'],
Expand All @@ -18,7 +18,7 @@ public static function asArrayProvider()
];
}

public static function asIteratorProvider()
public function asIteratorProvider()
{
yield ['D'];

Expand All @@ -27,7 +27,7 @@ public static function asIteratorProvider()
yield ['F'];
}

public static function asTraversableProvider()
public function asTraversableProvider()
{
return new WrapperIteratorAggregate([
['G'],
Expand Down
27 changes: 27 additions & 0 deletions tests/unit/Util/TestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,33 @@ public function testMultipleYieldIteratorDataProviders(): void
$this->assertEquals(3, $cCount);
}

public function testWithAbstractVariousIterableDataProviders(): void
{
$dataSets = Test::getProvidedData(\VariousIterableDataProviderTest::class, 'testAbstract');

$this->assertEquals([
['A'],
['B'],
['C'],
['D'],
['E'],
['F'],
['G'],
['H'],
['I'],
['J'],
['K'],
['L'],
['M'],
['N'],
['O'],
['P'],
['Q'],
['R'],

], $dataSets);
}

public function testWithVariousIterableDataProviders(): void
{
$dataSets = Test::getProvidedData(\VariousIterableDataProviderTest::class, 'test');
Expand Down