PHP CS Fixer: use PHPUnit Migration ruleset #25556
Conversation
| @@ -8,6 +8,8 @@ return PhpCsFixer\Config::create() | |||
| ->setRules(array( | |||
| '@Symfony' => true, | |||
| '@Symfony:risky' => true, | |||
| '@PHPUnit48Migration:risky' => true, | |||
| 'php_unit_no_expectation_annotation' => false, // part of `PHPUnitXYMigration:risky` ruleset, to be enabled when PHPUnit 4.x support will be dropped, as we don't want to rewrite exceptions handling twice | |||
There was a problem hiding this comment.
is the comment accurate? shouldn't we remove it?
I mean: I'm not sure we're going to move to "annotations" first, since using them works fine and is quite common in the code base
There was a problem hiding this comment.
rule is not about move "to", it's about move "away" from annotations.
annotations were already marked as not best practice, as it's better to reference by code to given class and to mark what is expected to raise exception (so one can say "expecting exception from this line" instead "anywhere in whole test method")
so, it is recommended to use ->setExpected* methods (which were removed on v6)
then, almost 2 years ago best practice was challenged again and changed to what we know from phpunit 5+ (->expect*) (not available on v4, note sebastianbergmann/phpunit#2931)
please take a note: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices
for that, it's part of migration ruleset, excluding rule out of the blue is always weird, it's better to describe why - I put a comment.
If you don't see it's usefull, feel free to drop it
about "it's quite common in codebase" - that's why we have automate for that kind of changes :)
There was a problem hiding this comment.
we're still maintaining branch 3.4 for a few years, and it runs on 5.5, so I'm not sure we can migrate the code base to ->setExpected* at all, isn't it?
since we also don't want to generate numerous merge conflicts, we won't want to apply this to master/4.0
which means this comment is premature AFAIK, that's what I meant
There was a problem hiding this comment.
and this comment is exactly to say that thing - as we don't want to use ->setExpected*, which is a temporary solution, we excluded this rule even if it's part of PHPUnit migration ruleset.
I find it pretty useful to say why we extracting sth from ruleset. Yet, if you find it useless, simply drop it.
|
I closed #25551 also, but if there is anything from it that should be merged, please add it to this PR. |
|
then I simply have to revert #25551 in my PR. |
|
done |
|
Thank you @keradus. |
This PR was squashed before being merged into the 2.7 branch (closes #25556). Discussion ---------- PHP CS Fixer: use PHPUnit Migration ruleset | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | not related | Fixed tickets | n/a | License | MIT | Doc PR | n/a replaces #25553 Commits ------- a5bc7d6 PHP CS Fixer: use PHPUnit Migration ruleset
replaces #25553