Incremental check for coding standards in pull requests

[morozov]

The existing approach of running phpcs over the entire codebase on pull requests is not really helpful because:

1. The build stage always fails due to existing standard violations.
2. It's unlikely that they all will be fixed any time soon due to the number of them (looks like thousands).
3. Fixing some violations may be BC-breaking-ish (e.g. renaming protected methods/properties starting with an underscore).
4. There's a waste of time of developers and reviewers on manually finding and additionally fixing violations.
5. If not found/fixed, new violations are introduced. It's a vicious circle.
6. Without proper automation, many new contributors (including myself until recently) don't even know that the project has its own custom coding standards.

Incremental checks could be done using morozov/diff-sniffer-pull-request like following:

# assuming we're in the dbal repo with installed dependencies
$ wget https://github.com/morozov/diff-sniffer-pull-request/releases/download/3.1.1/pull-request.phar
$ php pull-request.phar doctrine dbal 2494

FILE: lib/Doctrine/DBAL/Driver/SQLSrv/SQLSrvStatement.php
-------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------
 203 | ERROR | [x] There must be a single space before a NOT operator; 0 found
 203 | ERROR | [x] There must be a single space after a NOT operator; 0 found
 210 | ERROR | [x] There must be a single space before a NOT operator; 0 found
 210 | ERROR | [x] There must be a single space after a NOT operator; 0 found
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------
--More--

On Travis CI, it could be a conditional build job which is only executed for pull requests. Some pull requests will inevitably fail (e.g. if they override a method starting with an underscore). Even in this case, I'd prefer to not allow this stage to fail since a pull request with a failed build still can be merged.

Anyone volunteers to implement?

[lcobucci]

Just one small comment, the checker should use the phpcs.xml.dist present in the repo and not only the doctrine standard. That's the default behaviour of phpcs.

[morozov]

@lcobucci this will be possible after I release a new version built with phpcs ^3.1 due to squizlabs/PHP_CodeSniffer/issues/1591. It will have to be run like this:

$ php pull-request.phar doctrine dbal 2494 --runtime-set installed_paths vendor/doctrine/coding-standard/lib

The standard and the rest of parameters will be taken from phpcs.xml.dist.

[belgattitude]

@morozov, I won't have time for few weeks but just to tell it will be super useful to me.

[morozov]

@belgattitude I think we all need some rest :)

[morozov]

Here's how developers can validate their code before pushing on github: https://asciinema.org/a/151458.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.