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

PHPCS only on changed files and lines #45

Closed kmgalanakis closed 3 months ago

kmgalanakis commented 3 months ago

Description of the Change

In this PR I'm introducing two new arg for the action that could force the linter to run only on changed files or on changed lines.

Closes #4

How to test the Change

Create a repo that uses the action and has PHPCS errors and warnings just like this one https://github.com/kmgalanakis/10upCSactionTester

Changelog Entry

Added - Arguments to allow the action to run the linter only on changed files or on changed lines

Credits

Props @kmgalanakis

Checklist:

kmgalanakis commented 3 months ago

Hello @faisal-alvi

Thank you for your review. The way you tested this PR was a little bit unorthodox for me in the beginning but then it all made perfect sense. I haven't thought of testing it this way, so thank you for that. In fact, I created a fork of my own and followed the same testing process as you in this PR.

The problem it wasn't working for you (and for me initially) was because of that error #2 that was thrown

Fatal error: Uncaught Error: Undefined constant 'T_TYPE_OPEN_PARENTHESIS' in /github/home/phpcsutils/PHPCSUtils/Tokens/Collections.php:565

This T_TYPE_OPEN_PARENTHESIS constant lead me to this changelog and considering that this error was thrown in a folder called PHPCSUtils I assumed that something was wrong with this repo that we are cloning in the entrypoint.sh file for the case of use of the 10up Default standard. It seems that the release of the 1.0.12 version was breaking the action. Specifically cloning the 1.0.11 version seems to have resolved my issues, see here. I assume that it will also resolve yours (and show you your PHP errors) if you re-run your failed pipelines.

Please let me know what you think.

Thank you

kuldeep3 commented 2 months ago

@faisal-alvi Can we have this in the release. It would be really helpful.

jeffpaul commented 2 months ago

@kuldeep3 we'll likely get a release out in June, stay tuned!

kuldeep3 commented 2 months ago

Also @jeffpaul I tried with using the develop branch and used the WordPress as the standard, but it looks like it is unable to install the dependencies via github actions. Getting the "WordPress" coding standard is not installed. However, if I use Wordpress-VIP-Go, it was working fine but doesn't catching any errors.

faisal-alvi commented 2 months ago

@kuldeep3 Thanks for the report. We have a fix in place at https://github.com/10up/wpcs-action/pull/47.

faisal-alvi commented 2 months ago

@kuldeep3 The fix #47 is merged into develop branch, can you please check again and confirm if the issue is resolved? I tested at https://github.com/faisal-alvi/wpcs-action/pull/1/

kuldeep3 commented 2 months ago

Thanks @faisal-alvi I checked and it is now installing correctly. Another thing I noticed is the checks do not capture, indentation - like WP uses indentation of 4 tabs, if I use it as a 2 Spaces, it passes the check. Ideally, it should capture it. Moreover, on some ocassion, it doesn't even catches adding braces { to the next line while defining the function.

Let me know your thoughts, or do I need to use some other standards other than 'WordPress'

faisal-alvi commented 2 months ago

@kmgalanakis any thoughts on the above^ query?