Rector code quality #1648
Rector code quality #1648
Conversation
acoulton
left a comment
There was a problem hiding this comment.
@carlos-granados thanks for this - generally speaking all looks good with a few small comments.
IMO as this is changing actual code rather than just style / formatting, we should not ignore these changes from git blame. I am pretty sure these are safe, but I think it's a good convention that all functional code changes are blame-able.
Instead I would just squash merge this once complete (so the blame doesn't include intermediate changes where files have been modified by more than one rule).
| return array_reduce($transformations, function ($acc, $t) { | ||
| return [ | ||
| $t instanceof SimpleArgumentTransformation ? array_merge($acc[0], [$t]) : $acc[0], | ||
| !$t instanceof SimpleArgumentTransformation ? array_merge($acc[1], [$t]) : $acc[1], | ||
| $t instanceof SimpleArgumentTransformation ? $acc[1] : array_merge($acc[1], [$t]), | ||
| ]; | ||
| }, [[], []]); |
There was a problem hiding this comment.
This existing logic took a bit of figuring out! It would probably be easier to follow as a pair of simple array_filters (and if the array_reduce was originally a performance concern I'm not sure the repeated array_merge makes it any better).
But I agree this change is correct for the existing implementation.
src/Behat/Behat/Context/Snippet/Appender/ContextSnippetAppender.php
Outdated
Show resolved
Hide resolved
| ]) | ||
| ->withRootFiles() | ||
| ->withCodeQualityLevel(0) | ||
| ->withPreparedSets(codeQuality: true) |
There was a problem hiding this comment.
Are we at risk of CI breaking unexpectedly if Rector add new rules to the pre-defined "codeQuality" set that we have not yet adopted? We are only pinned to ^2.0, so every CI build will run with whatever happens to be the latest 2.x release of Rector at the time. I'm not sure their versioning strategy for changes to the config / implementation of rules?
We should perhaps be explicit about the rules - or alternatively require an explicit Rector version in composer.json and have a job (dependabot or a custom one like we have in behat/gherkin for the gherkin dependency) to update to each new Rector version as a standalone PR so that we can deal with any issues that arise without affecting other branches?
There was a problem hiding this comment.
I don't want to be explicit about the rules, because there are a lot to be listed, so instead what I have done is to pin the explicit version of Rector that we are requiring. Since this tool is only used internally for Behat development, we can keep updating the required version when we add more Rector rules
There was a problem hiding this comment.
That's a good solution.
Probably still worth having dependabot bump it so it doesn't fall too far behind (which could theoretically start holding back other dependencies in CI) but we can tackle that separately.
# Conflicts: # src/Behat/Testwork/PathOptions/Printer/ConfigurablePathPrinter.php
7a320ea to
eaf90e8
Compare
|
@acoulton sorry I took so long to get back to this. Summer is complicated here, hoping to get back to speed in September 😄 |
acoulton
left a comment
There was a problem hiding this comment.
Great, thanks @carlos-granados - and no worries, summer is busy here too 😁
Updates the Rector settings to apply the whole set of Rector code quality rules. Most of these rules do not produce changes in our code. For those rules that produce changes I have created one commit per rule to make this easier to review. Once this is fully reviewed, I will modify the last commit to include the git blame ignores for all these commits, please do not merge until this is done