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.

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

Merged
merged 1 commit into from Apr 28, 2021
Merged

DX: remove PHPStan exceptions for "tests" from phpstan.neon #5661

merged 1 commit into from Apr 28, 2021

Conversation

kubawerlos
Copy link
Contributor

@kubawerlos kubawerlos commented Apr 25, 2021

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.902% when pulling 7d32ab2 on kubawerlos:dx_phpstan_exceptions_for_tests into c2f6aa3 on FriendsOfPHP:2.18.

@@ -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`
Copy link
Member

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;

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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']);
Copy link
Member

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?

Copy link
Contributor Author

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.

@keradus keradus added this to the 2.18.7 milestone Apr 28, 2021
@keradus
Copy link
Member

keradus commented Apr 28, 2021

Thank you @kubawerlos.

@keradus keradus merged commit 8054f4c into PHP-CS-Fixer:2.18 Apr 28, 2021
@kubawerlos kubawerlos deleted the dx_phpstan_exceptions_for_tests branch April 28, 2021 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants