search

10up / wpcs-action

GitHub Action to help you lint your PHP without additional dependencies within your codebase
MIT License
54 stars 13 forks source link

Only run phpcs on changed files #4

Closed fabiankaegy closed 3 months ago

fabiankaegy commented 3 years ago

Describe your question

Is there an optoin that I am missing to only run the codesniffer against the changed files?

dinhtungdu commented 3 years ago

@fabiankaegy At the moment, no. But it's a useful feature to add.

fabiankaegy commented 2 years ago

@dinhtungdu @claytoncollie and I chatted about this during our open-source chat and discussed that it would be cool to either really only check the lines of code that were changed. Alternatively, it would be nice to not block the merging of the PR because of a failed phpcs check in a file or line that was not touched in the PR.

So maybe a check of the changed files vs the errors reported.

kmgalanakis commented 1 year ago

I assigned this to myself because I've previously done something similar on Gitlab CI.

Initially, I couldn't remember which package I had used so I started trying to approach this using git commands within the entrypoint.sh script, which apparently didn't work very well because I wasn't completely aware of what Github actions were doing with the checkout action. Then I thought that if I could get the list of modified PHP files in the PR I could potentially pass it to the paths parameter of the WPCS action and have PHPCS parse only those files. It wasn't hard to figure this out but on one of my tests, I realized that PHPCS was also detecting PHP issues on parts of the file that hasn't been changed. It was at that moment that I remembered which package I was using on Gitlab CI, it was PHPCS-Diff. I spent some time, figuring out how this could work without too much success yet.

I would like to keep this on me and try a couple more ideas I have approaching this.

kuldeep3 commented 5 months ago

Any updates on this one?

kmgalanakis commented 3 months ago

Hello @dkotter

@jeffpaul suggested that I ping you to confirm the ideal way to handle this, given that you are doing something similar on our WC client work.

In the past, I've used this action in projects hosted on Github but I'm not very familiar with building actions and I can't be sure if that action can be integrated into ours somehow.

Should I dig into that and figure this out?

Thank you

dkotter commented 3 months ago

@kmgalanakis

suggested that I ping you to confirm the ideal way to handle this, given that you are doing something similar on our WC client work

The approach that we've taken and implemented on a few client projects and are starting to bring into our OSS projects is ensuring any custom workflows we have only run when needed. For instance, no need to run PHPCS if a PR doesn't contain any PHP files; no reason to run eslint if the PR has no JS files; no need to run E2E tests if the PR only has readme updates; etc.

For the specific question here around PHPCS, the way we've handled that is approached in a few ways:

  1. We set our custom workflow to only run if there are files that end in .php
  2. If there is at least one PHP file, we then use the tj-actions/changed-files Action to get all files that are changed in that PR
  3. And finally we're using the phpcs-changed utility to only run PHPCS on the changed lines of code within the PHP files (instead of linting the entire file)

You can see this implemented on our Block Catalog plugin:

In the past, I've used this action in projects hosted on Github but I'm not very familiar with building actions and I can't be sure if that action can be integrated into ours somehow.

This is where I'm not exactly sure the best way to proceed. Not sure if we can easily integrate the above workflow into this custom Action, at least not without some refactoring. But if you're interested in digging into this some more, that would be great.

kmgalanakis commented 3 months ago

Thank you for your comment @dkotter

The approach that we've taken and implemented on a few client projects and are starting to bring into our OSS projects is ensuring any custom workflows we have only run when needed. For instance, no need to run PHPCS if a PR doesn't contain any PHP files; no reason to run eslint if the PR has no JS files; no need to run E2E tests if the PR only has readme updates; etc.

Please correct me if I'm wrong, but I think that this can be achieved by applying the relevant changes there where you define the action in the repo that uses it and not on the repo that defines the action.

In my implementation, I've taken bits and pieces from a similar implementation in this action and I think that the result is good and gets the job done. Feel free to test/review it here #45.

Thank you