Streamline Core sync workflow by improving coding standards linting and automation

[getdave]

During releases (and possibly elsewhere), identifying and fixing coding standards issues when syncing Gutenberg changes to WP Core are costing contributors a lot of time.

This is primarily down to the fact that we have two repositories (Gutenberg and WP Core) and thus standards can easily become misaligned between the two.

I believe that manually identifying and resolving these inconsistencies in individual PRs is a misuse of contributor time which could be better spent reviewing more impactful issues surrounding code structure/architecture.

That isn't in any way to downplay the importance of coding standards, but rather to observe that these kinds of issues should be either:

• fixed automatically by tooling
• automatically flagged for human review by linting

Example of problem

As an example, for a given sub-release of WP Core (for example an Release Candidate release), Editor Tech leads must:

• prepare and merge a PR in the Gutenberg repo with all the changes cherry picked for that release.
• publish the @wordpress/* packages.
• manually raise a Core PR with the synced package updates

All of this takes time, despite the levels of automation increasing with every release.

If, at the final step of reviewing the Core PR, coding standards issues are identified then a fix must be made upstream in the Gutenberg repo, before this entire package release process must be repeated.

Accounting for time taken for reviews and tests to pass, this process is extremely laborious and costly (both in terms of time and effort). This can lead to release timings being compromised.

This exact issue was flagged by @youknowriad during the 6.5 release:

The time loss for these things is huge...
Having linter helps catch these at the source and free the reviewers from paying attention to this.

Current Status

My (somewhat incomplete) understanding of the current state of the issue is:

• Core would like to enable linting on PRs but the linter ruleset is incomplete.
• Core would prefer to have a single "giant commit" fixing all code standards rule violation identified by a "complete" linter, rather than multiple large commits being raised as new rules are added to an enabled but incomplete linter.

Existing efforts

• An effort to add the WordPress-Docs standard to the PHPCS configuration - this has been abandoned.
• Update PHP codebase per WordPress PHP Coding Standards
• Gutenberg contributors have successfully automated all coding standards violations on the JS side of things. This was an incremental process and we may be able to take learning from this.

Questions

As co-Core Editor Tech for 6.5, I'm raising this discussion in the hope it can act as a point of focus to allow us to improve the situation for future releases.

I'd like to start things off by encouraging us to answer a few fundamental questions:

• What would the perfect scenario for removing manual code standards PR review look like? How would it feel to submit a Gutenberg -> WP Core sync PR in this world?
• What is specifically blocking us from making improvements today in:
  • WP Core
  • the Gutenberg repo
• What actions can we take to unblock this effort (in both Core and Gutenberg):
  • right now (6.5 cycle)?
  • during the 6.6 cycle?
  • in the future?

[youknowriad]

What would the perfect scenario for removing manual code standards PR review look like?

The perfect scenario is that linting and code styles should be handled automatically by the tooling. It shouldn't fall on the reviewers to take a look at this. We should focus the reviews on the actual feature, testing, and architecture.

We already do this for most code style things (almost 100% in JS code) but we still have some gaps on the php side, so let's to close these gaps. A good first step could be to enable the PHP Docs Linting on the Gutenberg repo (potentially before Core).

[anton-vlasenko]

If PHP Docs linting refers to the WordPress-Docs ruleset, it is not enabled in WordPress Core and has been removed from Gutenberg to ensure uniform standards between WordPress Core and Gutenberg, i.e., to simplify the backporting process.
It can be re-enabled in Gutenberg once WordPress Core adopts it.
The accepted proposal, along with all the reasoning, can be found here: #56487.

[getdave]

Thanks for that context. Very helpful as I wasn't aware.

It can be re-enabled in Gutenberg once WordPress Core adopts it.

I see this as a technical blocker that needs resolving. The inability of WP Core to adopt the WordPress-Docs rule is requiring developers to manually parse commits for coding standards violations.

What specifically is blocking adoption? My understanding is:

• large codebase makes it hard to adopt a linter that will throw warnings across legacy code
• the ruleset is incomplete and does not provide full coverage for the full coding standards

It might be gross simplification but it would seem to be the solution is to allow for a policy of gradual adoption. Some points to consider:

• accept that we don't need full coverage for all coding standards rules to adopt the docs linter. Even if a linter catches 80% of docs issues it's better than the current situation.
• identify where the existing sniffs in the ruleset do not match coding standards and adjust as required. All currently supported rules should match and support what is defined in the handbook. Note this does not mean we need to add new rules just ensure current ones adhere to the standards correctly.
• devise a means to enable the linter to behave selectively to enable gradual adoption:
  • apply error only for new code.
  • apply warning only for existing code.
• start a complementary effort to update the WordPress-Docs ruleset to provide coverage for all coding standards rules.

What's blocking this? What did I miss or misunderstand?

[jrfnl]

What's blocking this ?

From the PHPCS/sniff side of things the answer is fairly straight-forward:

WordPress has defined a number of docs coding standards (like docblocks for Hooks, a WP native format for array annotations and a few more things) which are so counter to the industry standard that there are no sniffs available to check these things, but moreover, that PHPCS native sniffs for docs issues which are available will break due to those counter-industry-standard choices.

This means that, in practice, only a very limited set of docs related checks can be enforced, which is what WordPress-Docs does to the best of its ability.
As-is, the WordPress-Docs standard should be able to enforce what can currently be enforced with the sniffs currently available.

Compounding the problem is that a lot of the current PHPCS native Docs sniffs are complex (they do too much), which makes them hard to configure and even harder to update.

To fix this, basically a complete new set of Docs sniffs need to be written, which is non-trivial and not to be under-estimated.

This is in the planning, but does not currently have priority, as on the PHPCS side there are more important things to do currently than to help WP fix their Docs standard.

devise a means to enable the linter to behave selectively to enable gradual adoption:

• apply error only for new code.
• apply warning only for existing code

That is not how PHPCS works and it never will.

As for why Core has not adopted the use of the WordPress-Docs standard (though they should): the sheer amount of issues (> 15000) to be (mostly manually) fixed before the ruleset can be turned on is a blocker.

To qualify that: the fast majority of these issues (>13.000) are in the Core test suite, which is sorely lacking appropriate docs.

A conceivable path forward could be to first turn the Docs ruleset on for WP src and only as a second stage, turn the Docs ruleset on for the tests as well.

I'd say, a good first step would probably be helping WP Core to fix the docs issues identified to allow the Docs standard to be turned on for WP Core. That should at least prevent a lot of issues with the docs between GB and Core not matching.

If, during that exercise, issues with the Docs ruleset are discovered, those can be reported to the WordPressCS project and we can try to accommodate, either by turning a certain rule off (for the time being) or, if possible, via a fix to one of the sniffs.

Note: while discussing the process for the GB to Core merges is appropriate here, discussing changes to the codified coding standards and how they can be improved, is not and any such discussion should be moved to the WordPress/WordPress-Coding-Standards repo.

[getdave]

Thank you for all that context. It's extremely helpful to those of us who might not have been a close to this work beforehand.

A conceivable path forward could be to first turn the Docs ruleset on for WP src and only as a second stage, turn the Docs ruleset on for the tests as well.
I'd say, a good first step would probably be helping WP Core to fix the docs issues identified to allow the Docs standard to be turned on for WP Core. That should at least prevent a lot of issues with the docs between GB and Core not matching.

This is what I'm keen to discover - practical ways in which we can move forward and improve.

Can I confirm that you're suggesting we raise a PR on the WP Core repo which enables the ruleset and then that would allow us to perceive the "issues identified" in /src so that we can fix them?

[jrfnl]

@getdave

It's extremely helpful to those of us who might not have been a close to this work beforehand.

Well, a little research before opening the issue could have told you nearly all of this....

Can I confirm that you're suggesting we raise a PR on the WP Core repo which enables the ruleset and then that would allow us to perceive the "issues identified" in /src so that we can fix them?

No, that's not what I'm suggesting as that PR cannot be accepted until all the issues have been fixed.

You can turn the ruleset on by using a local overload ruleset and submit patches to fix issues flagged to the WP Core Docs issue.

[getdave]

I'm going to answer my own question:

What would the perfect scenario for removing manual code standards PR review look like? How would it feel to submit a Gutenberg -> WP Core sync PR in this world?

When I write PHP code in the Gutenberg repo linting should enforce the same coding standards as required when I submit the patch to sync the changes to WP Core.

A few example that commonly come up:

• docblocks should be required
• all annotations should be enforced
• naming of classes/functions should be correct and standardised
• comments should utilise the correct format (i.e. /** rather than //).

The linter should be an aid to ensuring that any PHP code in Gutenberg trunk meets WP standards.

Then when I come to sync the changes to WP Core:

• I should see that there are no coding standards violations.
• Any that are uncovered should be caught by automated linting and flagged as "needs resolution" within the Github UI.\
• I should not need human reviewers to leave "coding standards" reviews.

Bottom line: the experience should feel seamless with automation guiding me towards ensuring that no human should need to spend their valuable time requesting changes to align with coding standards. All reviews should be focused on architecture or code decisions.

[swissspidy]

Totally agreed. I've also seen many core commits in the past for shared code for fixing some linter warnings that are not actually present core but only in Gutenberg. We should really have the same linter rules, simple as that.

[getdave]

We should really have the same linter rules, simple as that.

Whilst I agree, it's my understanding that unfortunately it isn't as simple as we'd both hope 😅 See the notes under Existing efforts above.

[anton-vlasenko]

Personally, I see two issues here:

1. Developing the missing linters to enforce WordPress Core standards.
2. Fixing an issue with synchronizing the rulesets between Gutenberg and WordPress to ensure a unified set of linters.

Therefore, in my opinion, these are two distinct issues, but both need to be addressed.

[swissspidy]

First time I'm seeing this discussion. SInce this affects core just as much as it affects Gutenberg, have you considered bringing up this topic in core dev chat or in a make/core blog post?

[getdave]

Yes sure. I was thinking of collecting some opinions here and then possibly following up with a Make Core post.

Happy to get this on the agenda for a Core Dev chat though.