Coding standards v3 #70
Conversation
There was a problem hiding this comment.
This is great. I have some suggestions in the comments. I also have one concern related to stability:
Firstly
If we're including whole upstream rulesets indiscriminately (as opposed to rule-by-rule), we run the risk that upstream changes and additions will cause sudden, unexpected failures when projects update their dependencies.[1] We could pin to specific versions to avoid that, but then we expose our users to version conflicts. I wonder if we shouldn't do this:
- Literally copy the entire upstream rulesets into ours so that we can release breaking changes on our own schedule.
- Create separate "Edge" standards for Drupal and PHP and add new upstream rules there so that changes are completely predictable.
- Possibly create a CI job to detect upstream additions so we're sure to notice and add them.
Secondly
As to whether we should rename these to public/private, that's a different way of thinking about the whole distinction than we've used in the past. We previously implied that "Strict" was better if you could manage it, whereas now it seems like we're saying they're actually appropriate or inappropriate based on context. I'm fine with the shift, but public vs. private doesn't seem quite right to me. It seems more like open source vs. proprietary. This represents late developing on my part and conflicts with some of my comments on the actual code, but I think it might actually address some of the discomfort I was expressing there.
[1] Unless all of our upstream dependencies carefully observe Semver, which we can scarcely hope for, much less depend on.
README.md
Outdated
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) is based on PSR-12 and is intended for use on all public non-Drupal projects. | ||
| * [AcquiaPHPStrict](src/Standards/AcquiaPHPStrict/ruleset.xml) is based on AcquiaPHP and adds additional more opinionated standards. It is intended for use on all internal non-Drupal projects. | ||
| * [AcquiaDrupal](src/Standards/AcquiaDrupal/ruleset.xml) is based on the Drupal coding standard and is intended for use on all public Drupal projects. | ||
| * [AcquiaDrupalStrict](src/Standards/AcquiaDrupalStrict/ruleset.xml) is based on AcquiaDrupal and adds the more opinionated DrupalPractice standard. It is intended for use on all internal Drupal projects. |
There was a problem hiding this comment.
Could we sort this list in the same order wherever it appears? It would make sense to me to either sort it in ascending order of framework-specificity and strictness or just alphabetically.
|
|
||
| * [AcquiaPHP](src/Standards/AcquiaPHP/ruleset.xml) contains sniffs applicable to all PHP projects. | ||
| * [AcquiaDrupalStrict](src/Standards/AcquiaDrupalStrict/ruleset.xml) incorporates AcquiaPHP and adds all Drupal coding standards and best practices sniffs. Recommended for new Drupal projects and teams familiar with Drupal coding standards. | ||
| * [AcquiaDrupalTransitional](src/Standards/AcquiaDrupalTransitional/ruleset.xml) incorporates AcquiaPHP and adds Drupal core's own phpcs configuration, which is less strict than the official standards. Recommended for legacy Drupal codebases or teams new to Drupal coding standards. | ||
| * [AcquiaEdge](src/Standards/AcquiaEdge/ruleset.xml) incorporates AcquiaPHP and adds backwards-incompatible sniffs that will be included in AcquiaPHP with the next major release of this package. |
There was a problem hiding this comment.
Maybe we should briefly explain how to use this. For example, should it be periodically be run manually? Should it be run separately on CI and allowed to fail? I haven't even really thought about this, to be honest--which might actually mean it would be better as a conversation for another time. ¯\_(ツ)_/¯
There was a problem hiding this comment.
It's a good question but probably out of scope for this README. More relevant to ORCA / RCAT I'd think?
As far as a consumer is concerned, I'd only expect early adopters like myself to use it.
Honestly... maybe we should just kill AcquiaEdge? If I'm the only early adopter that every used it, I can track standards in other ways.
| name="AcquiaDrupalTransitional" | ||
| name="AcquiaDrupal" | ||
| > | ||
|
|
||
| <description>Acquia's transitional Drupal coding standards.</description> | ||
| <description>Acquia's Drupal coding standards.</description> |
There was a problem hiding this comment.
The apparent lack of symmetry of AcquiaDrupal with AcquiaDrupalStrict makes me wonder if it doesn't need a modifier still. When you read it on this line, you see that we actually lose meaning--we don't just change it--by the omission of an adjective.
On a more pragmatic level (to reason from effects rather than semantics), it's always helpful for text searches to not have one concept not be represented by a sub-string of another concept. It's an imperfect example, but if you wanted to find every mention of AcquiaDrupal in the codebase, unless you used advanced search features, you'd also find every instance of AcquiaDrupalStrict.
What would you think of one of these adjectives?
- Basic
- Standard
- Foundational
- Minimal
There was a problem hiding this comment.
I like basic or minimal. How about:
- AcquiaPHPMinimal
- AcquiaPHPStrict
- AcquiaDrupalMinimal
- AcquiaDrupalStrict
My original intention was to mirror Drupal and DrupalPractice, but who outside the Drupal community would understand that pattern or what "practice" means 🤷
| <!-- Drupal Practice sniffs --> | ||
| <rule ref="DrupalPractice.Commenting.ExpectedException"/> |
There was a problem hiding this comment.
It's kind of curious to have only and exactly one rule from a stricter standard. I know it was already here, but the seeming incongruity is really highlighted now. (It's the only case where we tighten the requirements instead of loosening them.) Should we remove it? Or otherwise add an explanatory comment so future users don't have to search the codebase to figure out what it does like I did? 🙂
| @@ -8,11 8,4 @@ | |||
|
|
|||
| <description>Acquia's Edge (backwards-incompatible) coding standards.</description> | |||
There was a problem hiding this comment.
Do we need to break this into AcquiaPHPEdge and AcquiaDrupalEdge? If it's really inclusive of both, I foresee the same conceptual conflicts here that we're eliminating everywhere else by this refactoring. See my overall review comment for more detailed thinking.
There was a problem hiding this comment.
Yeah. Let's resolve the overall discussion of whether we want to delete the Edge standards first.
| <severity>0</severity> | ||
| </rule> | ||
| <rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing"> | ||
| <!-- PSR-12 has no opinion on comments, leading to unpredictable indentation. --> |
There was a problem hiding this comment.
PSR-12 has no opinion on comments, leading to unpredictable indentation.
Therefore... ? I can't tell from looking at the name what ignoreIndentationTokens = array does, so it's not clear what this does to help.
There was a problem hiding this comment.
Sorry... specifically, the problem is this. When you upgrade from v2 to v3 and run cbf, literally every line of your project will change from 2-space indents to 4-space indents, except inline comments, because PSR-12 explicitly ignores comment spacing/styling using the ignoreIndentationTokens array. So you get bastardized code like this:
function foo() {
// Call method bar().
$this->bar();
}By redeclaring this as an empty array, we persuade cbf to treat inline comments like all other code and indent them accordingly.
I wasn't sure how to express that succinctly...
Co-authored-by: Travis Carden <[email protected]>
The goal of v3 is to simplify and decouple the standards, as described in the readme:
To do:
References: