Implement proposal to remove the WordPress-Docs ruleset #56982
Conversation
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
|
Flaky tests detected in cbfc228. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7199385494
|
anton-vlasenko
requested review from
hellofromtonya,
azaozz,
ironprogrammer and
aaronjorbin
anton-vlasenko
added
the
[Type] Code Quality
hellofromtonya
added
the
Gutenberg Plugin
| @@ -1651,7 +1651,7 @@ private function apply_attributes_updates( $shift_this_point = 0 ) { | |||
| * replacements adjust offsets in the input document. | |||
| */ | |||
| foreach ( $this->bookmarks as $bookmark_name => $bookmark ) { | |||
| $bookmark_end = $bookmark->start + $bookmark->length; | |||
| $bookmark_end = $bookmark->start + $bookmark->length; | |||
There was a problem hiding this comment.
This change is out-of-scope for this PR. Need to do it in a separate issue/PR please.
There was a problem hiding this comment.
Thank you for the review!
I would say it's related to some degree because the following exclude pattern was added to phpcs.xml.dist to prevent these files from being checked against the WordPress-Docs ruleset:
<exclude-pattern>./lib/compat/wordpress-*/html-api/*.php</exclude-pattern>If WordPress-Docs is disabled, the <exclude-pattern> statement above is no longer needed.
However, I don't mind submitting another pull request to address that specific issue. I will revert this now.
There was a problem hiding this comment.
Fixed in ae3f74476b4ce9bddcc82d9e2572826650624e4d.
There was a problem hiding this comment.
I see this out of scope because the alignment of = in the source code is not part of the WordPress-Docs ruleset.
There was a problem hiding this comment.
I'd suggest making a PR for that alignment issue and then once merged into trunk, then rebase this PR to include that change.
There was a problem hiding this comment.
was there a plan to add the rule back in that was removed for the html api? this caused another hassle with @SergeyBiryukov's change that started blocking backports into Gutenberg.
was it even intentional that the HTML API exclusion was removed?
b592ba9#diff-05ae9cddcaec1e845771a7db224961439f83ef5939ec67d3a48744cb34d7e58bL62-L63
should we add that exclusion back in?
There was a problem hiding this comment.
was it even intentional that the HTML API exclusion was removed?
@dmsnell
Yes, it was 100% intentional.
Personally, I'd only add exceptions for conflicting rules (i.e. the ones that aren't enabled in Core but are enabled in Gutenberg), like VariableAnalysis.CodeAnalysis.VariableAnalysis.
should we add that exclusion back in?
If it makes things easier, please add it back.
There was a problem hiding this comment.
thanks. I'll do that and add you as a reviewer. given how frequently it seems that the rules change, and how people rush through changes without review that end up introducing challenges I'd rather we treat the HTML API compatibility layer just like we treat NPM packages, which is why we added this exclusion originally.
thanks for your guidance on this.
is there anything we can do to alert someone not to remove the rule again? I originally thought that its very nature of existing as an exclusion would prevent it, but would a large "DO NOT REMOVE" warning help? I prefer to avoid those if possible.
There was a problem hiding this comment.
thanks for your guidance on this.
You are welcome.
is there anything we can do to alert someone not to remove the rule again? I originally thought that its very nature of existing as an exclusion would prevent it, but would a large "DO NOT REMOVE" warning help
Perhaps the comment should explain why this exclusion was made so that people who don't know the context wouldn't remove it. Something along the lines of, Do not remove this exclusion, as it is made for compatibility reasons; Gutenberg and Core PHPCS standards are not fully aligned at the moment.
What do you think?
There was a problem hiding this comment.
thanks. I'll try and be more obtuse in the language. When this was originally removed in b592ba99 it read…
<!-- Exclude files maintained in WordPress Core and backported to Gutenberg -->I've updated it in #57922 to read…
<!-- Exclude files maintained in WordPress Core and backported to Gutenberg
DO NOT REMOVE these rules; these files are "built" artifacts from Core
and when they are removed it prevents keeping the repos in sync. -->Hoping this does a better job.
anton-vlasenko
force-pushed
the
try/remove-wordpress-docs-coding-standard
branch
from
aeb0207 to
9094632
Compare
anton-vlasenko
force-pushed
the
try/remove-wordpress-docs-coding-standard
branch
from
9094632 to
cbfc228
Compare
What?
This pull request removes the
WordPress-Docsruleset from Gutenberg with the goal of simplifying the backporting process.Fixes #56487.
Why?
Also, please read the proposal as it explains this in more detail.
How?
WordPress-Docsruleset from thephpcs.xml.distfile.Squiz.Commenting*andGeneric.Commenting.DocComment*rules as they are added by theWordPress-Docsruleset and are no longer needed.phpcs.xml.distfile (those changes are no longer needed).Testing Instructions
Review the changes and make sure that the Github CI jobs pass.
Testing Instructions for Keyboard
Screenshots or Screencast