rtCamp / action-phpcs-code-review

Github Action to perform automated code review on pull requests
https://github.com/rtCamp/github-actions-library
MIT License
103 stars 27 forks source link

Scan files being erroneously scanned #35

Open dannydover opened 4 years ago

dannydover commented 4 years ago

First off, thank you for building this! This has been working great for me and is really helping my codebase stay up-to-par.

However, I am running into one reoccurring issue. About 1/4 of the time when this runs automatically after opening a pull request, I receive the following error:

šŸš« Error: Filenames should be all lowercase with hyphens as word separators. Expected phpcs-scan-ysewvc.php, but found phpcs-scan-Ysewvc.php (WordPress.Files.FileName.NotHyphenatedLowercase).

"phpcs-scan-Ysewvc.php" -- As you might expect, the filename changes each time this occurs (characters after the second hyphen differ):

Is there a way to either ignore these phpcs scan files or fix the root cause of this issue?

For context, I have included my workflow file below:

# Original Documentation https://github.com/rtCamp/action-phpcs-code-review

on: pull_request

name: Code Quality
jobs:
  runPHPCSInspection:
    name: Run PHPCS linter
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}
    - name: Run PHPCS inspection
      uses: rtCamp/action-phpcs-code-review@v2.0.2
      env:
        GH_BOT_TOKEN: ${{ secrets.GH_CICD_BOT }}
        SKIP_FOLDERS: "tests,.github,cypress"
      with:
        args: "WordPress,WordPress-Core,WordPress-Docs"
mrrobot47 commented 4 years ago

Please refer: https://github.com/rtCamp/action-phpcs-code-review/issues/34

NickGreen commented 4 years ago

Thanks for making this issue, I'm getting the same problem:

šŸš« Error: Filenames should be all lowercase with hyphens as word separators. Expected phpcs-scan-xe6at7.php, but found phpcs-scan-xe6aT7.php (WordPress.Files.FileName.NotHyphenatedLowercase).

Is there anything I can do to help troubleshoot or resolve this?

mrrobot47 commented 4 years ago

I will be looking into this issue over the weekend or in the next week. Will loop you in by Monday or Tuesday with more details :slightly_smiling_face:

dannydover commented 4 years ago

@mrrobot47 Thank you! That is very much appreciated it.

NickGreen commented 4 years ago

@mrrobot47 Sorry to bother you - I appreciate all the work you've done on this. I'm having problems with the workaround of ignoring the WordPress.Files.FileName.NotHyphenatedLowercase rule.

I want to use the WordPress-Extra ruleset, which includes the WordPress-Core rules, but I think I'm excluding that specific rule wrong. Here's my phpcs.xml rule declaration:

    <rule ref="WordPress-Extra">
        <exclude name="WordPress.Files.FileName.NotHyphenatedLowercase" />
        <exclude name="WordPress.Files.FileName.InvalidClassFileName" />
    </rule>

~However, I'm still occasionally getting the error above, and my current theory is I have to ignore that rule specifically from the WordPress ruleset? Any ideas?~

Disregard - I figured it out. The phpcs.xml file has to be included in the branch that is being tested - or it won't know to look at it. So, if you've added the xml file to the master branch, then make sure to pull it into the branch that you need to test.

mrrobot47 commented 4 years ago

Sorry for replying late on this issue. I got caught up on other work and could not check on this.

The issue of WordPress.Files.FileName sniff is due to how the temp files are created having the diff of the PR. tempnam function creates file with the name that could also contain uppercase letters. This issue needs more discussion on repo vip-go-ci, will create an issue for the same and discuss it there later. Or you can initiate the same and reference this issue.

For now, I have added the option to skip sniffs using env var: PHPCS_SNIFFS_EXCLUDE, this is the equivalent of --exclude flag of phpcs. This works in both cases, when you have phpcs.xml file in repo, or only have sniffs listed as args in the workflow file.

Update your workflow file according to the latest release usage having the PHPCS_SNIFFS_EXCLUDE env var and you will not face this error again:

on: pull_request

name: Inspections
jobs:
  runPHPCSInspection:
    name: Run PHPCS inspection
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}
    - name: Run PHPCS inspection
      uses: rtCamp/action-phpcs-code-review@v2.0.3
      env:
        GH_BOT_TOKEN: ${{ secrets.GH_BOT_TOKEN }}
        SKIP_FOLDERS: "tests,.github"
        PHPCS_SNIFFS_EXCLUDE: "WordPress.Files.FileName"
      with:
        args: "WordPress,WordPress-Core,WordPress-Docs"
NickGreen commented 4 years ago

Sorry for the late response on this.

Update your workflow file according to the latest release usage having the PHPCS_SNIFFS_EXCLUDE env var

Is adding this required, or will the check still work if that doesn't exist in the action?

I'm currently trying to track down an issue where the exclude patterns in the phpcs.xml file are being ignored.

Possibly related, this is showing in the action output:

Invalid PHPCS sniff(s) specified in options or options file (option 'phpcs-sniffs-exclude')

Likely because PHPCS_SNIFFS_EXCLUDE: is not defined in the action.

Will not having that defined affect the exclude rules in the phpcs.xml file?