Incremental check for coding standards in pull requests

[morozov]

WIP. It's unclear if we'll need the key to access the GitHub API and not exceed the limit.

Fixes #2917.

[Majkl578]

Can you share some examples of how this looks in real life?

It's unclear if we'll need the key to access the GitHub API and not exceed the limit.

Downloads are not part of API rate limiting, no rate limit headers are present (302 to AWS).

[morozov]

@Majkl578 see the most recent build failure.

If the entire file was checked, the report would look like:

phpcs lib/Doctrine/DBAL/Connection.php

FILE: lib/Doctrine/DBAL/Connection.php
------------------------------------------------------------------------------------------------
FOUND 45 ERRORS AFFECTING 40 LINES
------------------------------------------------------------------------------------------------
  202 | ERROR | [x] The first parameter of a multi-line function declaration must be on the
      |       |     line after the opening bracket
  202 | ERROR | [x] Multi-line function declarations must define one parameter per line
  202 | ERROR | [x] Multi-line function declarations must define one parameter per line
  203 | ERROR | [x] Multi-line function declaration not indented correctly; expected 8 spaces
      |       |     but found 12
  203 | ERROR | [x] The closing parenthesis of a multi-line function declaration must be on a
      |       |     new line
  204 | ERROR | [x] The closing parenthesis and the opening brace of a multi-line function
      |       |     declaration must be on the same line
  209 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 8 spaces but
      |       |     found 1 space
  228 | ERROR | [x] There must be a single space before a NOT operator; 0 found
  228 | ERROR | [x] There must be a single space after a NOT operator; 0 found
  236 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 7 spaces but
      |       |     found 1 space
  374 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 10 spaces
      |       |     but found 1 space
  375 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 6 spaces but
      |       |     found 1 space
  378 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 8 spaces but
      |       |     found 1 space
  448 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 12 spaces
      |       |     but found 1 space
  464 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 11 spaces
      |       |     but found 1 space
  471 | ERROR | [x] Blank line found at end of control structure
  486 | ERROR | [x] There must be a single space before a NOT operator; 12 found
  632 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 4 spaces but
      |       |     found 1 space
  633 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 5 spaces but
      |       |     found 1 space
  642 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 4 spaces but
      |       |     found 1 space
  643 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 5 spaces but
      |       |     found 1 space
  736 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
  737 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 8 spaces but
      |       |     found 1 space
  741 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
  742 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 8 spaces but
      |       |     found 1 space
  746 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 47 spaces
      |       |     but found 1 space
  747 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 48 spaces
      |       |     but found 1 space
  753 | ERROR | [x] Equals sign not aligned correctly; expected 1 space but found 2 spaces
  779 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
  780 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 5 spaces but
      |       |     found 1 space
  784 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
  785 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 5 spaces but
      |       |     found 1 space
  975 | ERROR | [x] There must be a single space before a NOT operator; 0 found
  975 | ERROR | [x] There must be a single space after a NOT operator; 0 found
  999 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 3 spaces but
      |       |     found 1 space
 1247 | ERROR | [x] Concat operator must be surrounded by a single space
 1552 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
 1556 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 22 spaces
      |       |     but found 1 space
 1568 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 22 spaces
      |       |     but found 1 space
 1592 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 7 spaces but
      |       |     found 1 space
 1620 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 2 spaces but
      |       |     found 1 space
 1624 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 23 spaces
      |       |     but found 1 space
 1625 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 15 spaces
      |       |     but found 1 space
 1636 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 18 spaces
      |       |     but found 1 space
 1637 | ERROR | [x] Equals sign not aligned with surrounding assignments; expected 10 spaces
      |       |     but found 1 space
------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 45 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------

Downloads are not part of API rate limiting, no rate limit headers are present (302 to AWS).

This script uses /repos/:owner/:repo/pulls/:number and /repos/:owner/:repo/contents/:path to get the diff and blobs accordingly.

According to the documentation, the encrypted key will not work for pull requests, so if there's a limiting problem, I'll have to rework the script to use the actual git repo clone, not the API (the original design didn't account for CI like Travis and didn't require the local repository clone).

[Majkl578]

I like this concept.
If I understand it correctly, there may be some false positives on the same line (i.e. user changes a message on a line where no spaces are present around dot).

We just need to find out how likely we're to hit this API limit, not really sure how to do it in a deterministic way.

[morozov]

If I understand it correctly, there may be some false positives on the same line (i.e. user changes a message on a line where no spaces are present around dot).

I wouldn't consider this particular case a false positive. If a line is modified, it should be formatted according to the standards, even if it's not modified entirely. This way we'll achieve better compliance without producing extra diffs.

Of course, some false positives are inevitable but we should be able to cope with them.

[morozov]

We just need to find out how likely we're to hit this API limit, not really sure how to do it in a deterministic way.

< Status: 200 OK
< X-RateLimit-Limit: 60
< X-RateLimit-Remaining: 49

It appears, for Travis, the limits are the same as for other non-authenticated clients. Unless there's a way to authenticate Travis to the API, we cannot use this approach. We'll need to get the diff and blobs from the local clone.

[ostrolucky]

This is really ineffective. Why use API when you can just do a git diff on travis?

[morozov]

@ostrolucky because it was designed long ago to not require a local clone. Depending on the repo and pull request, it may be much more efficient to just pull a couple of files using the API instead of cloning a repository with a ten-year-old history.

I've almost finished re-implementing it to use a local repo.

[ostrolucky]

Yes, I can see this useful for different use cases, not for a CI though. Good to hear you are working on more effective solution though! Here's some old code in CI we use, might help you

for file in `git diff-tree --no-commit-id --name-only --diff-filter={A,M} -r HEAD~1..HEAD`;
do
    cp $file /tmp/phpcs-test/
done
./vendor/bin/php-cs-fixer fix /tmp/phpcs-test --dry-run --diff --verbose --rules=@Symfony,-phpdoc_align,-phpdoc_summary,-simplified_null_return,-yoda_style,-self_accessor

[ostrolucky]

it may be much more efficient to just pull a couple of files using the API instead of cloning a repository with a ten-year-old history.

I don't have much experience with travis, but I believe all CIs work by cloning the project. In case of travis it seems to be clone with depth of last 50 commits. So you don't need to do additional clone. GIT repo should be available right there. That's why I'm saying this is really ineffective.

[morozov]

@Majkl578 @ostrolucky please see the updated patch and build failure.

[Majkl578]

LGTM 👍, and once we finish doctrine/coding-standard#9 this may be great for PRs until we enforce CS globally in the repo.

[Majkl578]

Looks like ORM is not going to need this after all, develop will be merged into master in upcoming days and we plan to apply CS globally on whole repo afterwards, followed by increasing PHPStan level. But it can still be useful here and other repos that don't yet adhere to our CS. :)

[Ocramius]

Careful here: travis clone operations are generally shallow, which means that this may fail after 100 commits or even less.

[morozov]

From the documentation:

Rather than test the commits that have been pushed to the branch the pull request is from, we test the merge between the origin and the upstream branch.

I was under the impression that both origin/$TRAVIS_BRANCH and $TRAVIS_PULL_REQUEST_SHA will be immediate parents of the merge commit. So they both should be on the second level of depth.

Not sure how shallow clone handles multiple parents though.

[morozov]

In theory, there indeed may be a problem when the merge base of the two parents is more than 50 commits behind any of them. So even if we fetch the merge base manually, there's no guarantee all commits in between are available. Hm…

[morozov]

@Ocramius what about this? The non-shallow clone of this PR's branch is ~18MB, not that bad and should be fetched only in edge cases.

[Ocramius]

Works for me 👌

[Majkl578]

By the way, Travis allows depth configuration or even disabling it: https://docs.travis-ci.com/user/customizing-the-build/#Git-Clone-Depth

[morozov]

@Majkl578 it does but a deep clone by default would be an overkill and we don't know the exact depth we need for a given pull request.

The current solution is a tradeoff between performance and simplicity. The default depth of 50 will be enough in 99?% of cases. In the rest, we fetch the missing commits.