Closed justlevine closed 10 months ago
To further this goal across the ecosystem (and a deeper explanation on why a shared ruleset is important), I released this WPGraphQL Coding Standards, which should make it significantly easier to evaluate the impact and value on this codebase.
To test:
composer require --dev axepress/wp-graphql-cs
locally to install the sniffs.phpcs.xml.dist.example
file over to the root directory and rename it to phpcs.xml
. phpcs.xml
with those from phpcs.xml.dist
vendor/bin/phpcs
and review the results (or even better run vendor/bin/phpcbf
, and then diff the branch).@justlevine Thanks. We do maintain a phpcs plugin
https://github.com/wpengine/wpengine-coding-standards
do you think we can include this ruleset there?
@theodesp looking at the ruleset, the sniffs WPEngine-Strict has cherry-picked are already included in WPGraphQL-Minimum
, as are all the ones in the phpcs.xml.dist
of this repo.
The only conflict I see is the wp-core preference for long array()
syntax - WPGraphQL core (and the ruleset) enforces the short []
syntax.
The next level up, WPGraphQL-Strict
includes functional sniffs about PHP best practices, so that also should be fine.
The one that gives me pause is the optional WPGraphQL-Extra
that handles formatting, alignment, and some extra doc-block requirements, as it's opinionated and might go against what the Faust team uses internally (although unenforced by the WPEngine ruleset). I did my best to use this repo as a guide for that (e.g. the no-space before a method's : ReturnType
goes against my personal preference and what WPGraphQL core currently does), considering that afaik, WPEngine has the largest number of paid contributors to the ecosystem. I'm sure the team will have feedback, and I'm looking forward to incorporating it 😁
Hey @justlevine has #111 satisfied this issue? Thank you!
@josephfusco when I reviewed it pre-merge, I noted that PR was actually justification in support of this issue, and as far as I can tell, the merged version didnt make any changes to the PHPCS ruleset.
A stricter (and ecosystem-shared) PHPCS ruleset would have prevented/autofixed many of those issues from leaking into the codebase. Further, it would catch additional smells missed by that PR (to be fair, last time I reviewed was pre-merge) , and most importantly it would keep future changes to the codebase clean so a housekeeping PR like #111 would be mostly unnecessary.
@josephfusco take a look at the individual commits in #126 . Beyond switching to modern array syntax, it caught (and in many cases fixed), typing smells, memory consumption from nonstatic callables.
To get a feel for its future usefulness, you can cherry pick the first commit onto a branch from before my and/or @mindctrl 's series of housekeeping PRs, and see how many would have been autofixed or at least caught before merge.
Closing this ticket since https://github.com/wpengine/wp-graphql-content-blocks/pull/165 is merged.
In order to make it easier for other developers in the ecosystem to contribute to this plugin,would it be possible to align with some of the ruleset options used by WPGraphQL core - or at least the ones that dont conflict too bad with the included WPEngine ruleset (I'm not particularly familar with what ya'll usually use internally)?
(I'd be particularly nice if in 2023 we could be all be using short array syntax).
Thanks for considering!