-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WPCS 3.0: Fix incompatibilities by using PHPCSUtils #734
Conversation
Tests pass fine locally, so not sure why GitHub Actions is having an issue finding and using PHPCSUtils. Edit: Log shows only phpcsstandards/phpcsutils (1.0.0-alpha3) was installed, not alpha4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed with the exception that I haven't done a detailed review of the last two commits (see below).
Along the same lines as my comment on #733:
- Most of this PR does not depend on PR WPCS 3.0: Composer updates #732. These changes can be made independently of the WPCS update (and in anticipation thereof).
- The first commit should be adding the PHPCSUtils package as a dependency as the other commits all depend on that package being available (the "Composer: Add PHPCSUtils as required dependency" commit).
- If commit 4 ("Add Sniff::merge_custom_array()") stays, I'd suggest squashing commit 6 ("CS: fix coding standards for merge_custom_array()") into that same commit.
- The last two commits ("CI: Refresh unit tests workflow" and "Disable ruleset test") do not belong with this PR.
I actually had a PR ready for most of those changes too, but got stuck on debugging why the ruleset test wasn't working, which is why I hadn't pulled it yet.
I'd recommend rebasing the PR (without those last two commits) on develop
and changing the commit order.
74bd1ea
to
db681cc
Compare
db681cc
to
4dd0ad7
Compare
Done. |
1144faa
to
5202918
Compare
VIPCS makes use of PHPCSUtils directly (such as for MessageHelper), so it should be marked as such in the Composer config.
This is more tested than the WPCS Sniff::addMessage().
5202918
to
7ce4c18
Compare
Once #742 has been merged, this can be rebased to allow the workflow tests to run. #742 makes PHPCS 3.7.1 as the minimum version for VIPCS, which is the constraint on PHPCSUtils 1.0.Now rebased.Several methods were removed or moved from the Sniff class in WPCS, so these changes were spotted when running unit tests and getting errors. There are undoubtedly many more opportunities to use PHPCSUtils versions of functions to replace existing VIPCS code to improve the quality of the sniffs, but these are not tackled here.
These changes don't require WPCS 3.0 - we're just making the same types of changes that WPCS 3.0, by using PHPCSUtils as a direct dependency.
Composer: Add PHPCSUtils as required dependency
VIPCS uses PHPCSUtils directly (such as for MessageHelper), so it should be marked as such in the Composer config.
Sniff::strip_quotes(): switch to use the PHPCSUtils version
The original
Sniff::strip_quotes()
method was removed for WPCS 3.0.Sniff::find_array_open_close(): switch to use the PHPCSUtils version
The original
Sniff::find_array_open_close()
method was removed for WPCS 3.0.Sniff::get_function_call_parameter(): switch to use the PHPCSUtils version
The original
Sniff::get_function_call_parameter()
method was removed for WPCS 3.0.Use PHPCSUtils MessageHelper::addMessage
This is more tested than the WPCS
Sniff::addMessage()
, and the original was removed.