This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Replace phpcs rule #11724
Closed
Replace phpcs rule #11724
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roykho
added
type: bug
woocommercebot
requested review from
a team and
albarin
and removed request for
a team
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.61 MB ℹ️ View Unchanged
|
Thanks for reaching out, @roykho. I see that with https://github.com/woocommerce/woocommerce-blocks/pull/5911/files, we introduced the following rule: <rule ref="WordPress.PHP.DisallowShortTernary.Found">
<exclude-pattern>src/*</exclude-pattern>
<exclude-pattern>tests/*</exclude-pattern>
</rule>Unfortunately, I cannot find any further information in the corresponding PR of why this rule had been added. @mikejolley as you authored that PR, do you recall why it had been added, and why the |
nielslange
reviewed
Nov 13, 2023
There was a problem hiding this comment.
Thanks for working on this, @roykho.
Currently, the testing instructions state:
- Have PHPCS installed globally.
- Install the WooCommerce Sniffs
composer global require woocommerce/woocommerce-sniffs --dev- [...]
In /package.json we do have this definition:
woocommerce-blocks/package.json
Line 67 in 4b5ba05
I wonder if it wouldn't be better to adjust the testing instructions accordingly to use that command. 🤔
Probably not in this case because running that command will lint everything which indeed will flag errors for unchanged files. That can make it seem something went wrong. We can however include a test like |
|
I've pushed up changes to bump |
Good idea! With the current version of |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
danielwrobert
force-pushed
the
phpcs/remove-rule
branch
from
c7d6e02
to
975671a
Compare
There was a problem hiding this comment.
I've run through the steps in the test plan and everything works as described. 🎉
However, as we've been discussing in Slack, the PHP Coding Standards / PHP coding standards (pull_request) check is still failing at the Run PHPCS on all changed files step.
Is it possible that it's failing because the rule has been updated and that subsequent runs will pass after this PR is merged? 🤔
|
Yeah because of the rule change, it runs the rule against all files it seems. I confirmed this via this test PR #12071 which is a dup of this PR here. Though looking at that specific action, I am not sure where it states to do that. |
|
Closing ahead of the monorepo merge and will recreate there. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
This PR replaces the removed
WordPress.PHP.DisallowShortTernaryin favor ofUniversal.Operators.DisallowShortTernary.Ref: https://github.com/WordPress/WordPress-Coding-Standards/blob/7722c0b8a6f4eb615be4ba8ebe22ed92a5fa87d7/CHANGELOG.md?plain=1#L231
Why
Since the rule is removed, it is causing an error such as this in the VS Code editor
Note
I am not sure why we're trying to exclude the
src/*folder since this is the folder where the bulk of the PHP files lives. I would think this is exactly the place we should lint? Perhaps you know cc @nielslangeAlso this is blocked by WooCommerce Sniffs need to be first updated to version1.0.0so we can use WP coding standards version^3.0.0.Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
composer global require woocommerce/woocommerce-sniffs --dev(version 1.0.0)which phpcsto find the path).WooCommerce-Corein thesettings.jsonlike"phpcs.standard": "WooCommerce-Core"./src/*and try to add a shorthand ternary operation and ensure PHPCS catches that. You can use$foobar = true ?: false;.Screenshots or screencast
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog