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.
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
DX: remove PHPStan exceptions for "tests" from phpstan.neon #5661
DX: remove PHPStan exceptions for "tests" from phpstan.neon #5661
Conversation
@@ -193,7 +193,7 @@ public function testRegisterCustomFixersWithInvalidArgument() | |||
$this->expectExceptionMessageMatches('/^Argument must be an array or a Traversable, got "\w+"\.$/'); | |||
|
|||
$config = new Config(); | |||
$config->registerCustomFixers('foo'); | |||
$config->registerCustomFixers('foo'); // @phpstan-ignore-line to avoid `expects iterable<PhpCsFixer\Fixer\FixerInterface>, string given` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of this change, it's moving issue handling from one place to another, while on 3.0 we already have the fix in ConfigInterface directly:
public function registerCustomFixers(iterable $fixers): self;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is not having such exceptions in phpstan.neon
- this is not error to ignore, but expected wrong usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point was that whatever you produce here, it is out on 3.0 line, because we enforced proper use on typehint level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not bad, is it? What's important to me it is out of phpstan.neon
file, so that file can be improved futher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm saying that I don't see the value of this change, because what you aim is to move the warning behind this line out of phpstan.neon file, but that warning is already removed in phpstan.neon file on 3.0 line: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/3.0/phpstan.neon
so, doing the change on 2.x is not bringing any benefit (we have better change already on 3.0), but only causing the conflict for 2.x->3.0 merge.
@@ -44,6 +44,6 @@ public function testResolveWithMappedRoot() | |||
|
|||
static::assertSame($options, $configuration->getOptions()); | |||
|
|||
$configuration->resolve(['baz', 'qux']); | |||
$configuration->resolve(['baz' => 'qux']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't goal of this test to have non-assoc array passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not - test is still passing. According to expected deprecation message it tests config for creating resolver, not resolving.
Thank you @kubawerlos. |
After #5626 it got me thinking that PHPStan is not always able to detect errors properly - sometimes because it runs on single PHP version with single set of dependencies, sometimes because there is some dynamic behaviour. So it may be not bad idea to use
@phpstan-ignore-line
as it's not really exception, but false positive case.Nonetheless, in this PR all the PHPStan exceptions are removed from phpstan.neon and fixed or handled via
@phpstan-ignore-line
annotation.