QA: "normalize" the code style rules used by this code base a little

[jrfnl]

The coding standard of this code base was historically/organically grown and is likely influenced by some internal rules which were being used in Squiz projects at one time or another.

Changing the code style to PSR12 with some extra rules (i.e. PHPCSDevCS) would cause lots and lots of code churn, so is not on the table at this time.

However, I do think there are some rules which can (and should) be changed in the near future.
On the one hand, these rules cause code churn on lines unrelated to the actual code change being made.
On the other hand, these rules catch contributors out a lot as they are contrary to what most code bases nowadays expect, making contributing to this code base more painful for new contributors than necessary.

My personal top 5 of "annoying" rules which are being enforced is:

1. End comments at the end of "long" control structures/functions/classes.
2. else if instead of elseif
3. No spaces around a concatenation operator.
4. The case/default statements in switch statements not being indented by four spaces - changing this would cause lots of code churn in the test files though.
5. Unnecessary blank lines at the end of each function/class.

I'm opening this issue to allow for people to voice their opinion on this.

• Do the above five rules bother you or not ?
• Are there other rules, not mentioned above, which catch you out often when contributing ?
• Do you think the code churn caused by changing these rules and updating the code base to comply is worth it ?

I'm going to leave this ticket open for a few weeks and will decide on a course of action and timeline after that.

/cc @fredden

[rodrigoprimo]

Do the above five rules bother you or not?

Yes, they bother me a bit. Especially items number 1, 4, and 5. I'm ok with changing items number 2 and 3 as well.

Are there other rules, not mentioned above, which catch you out often when contributing?

I'm not a fan of Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned as it causes changes on unrelated lines and causes larger diffs. Example: https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/179/files#diff-b9f974c9e5e2ca608e335ac55aa95730811bcaef7590ad41b4d59d5444f296c9R47 (only the highlighted line actually changed).

Do you think the code churn caused by changing these rules and updating the code base to comply is worth it?

I think so. Especially for rules that can be changed automatically using phpcbf. Using .git-blame-ignore-revs should help reduce the impact of the code churn.

[dingo-d]

I'm not a fan of Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned as it causes changes on unrelated lines and causes larger diffs. Example: https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/179/files#diff-b9f974c9e5e2ca608e335ac55aa95730811bcaef7590ad41b4d59d5444f296c9R47 (only the highlighted line actually changed).

The protip for reviewing code like this is to turn on the hide whitespace option (props to Juliette for mentioning this to me 😄 )

[jrfnl]

Another one to add to the "catches me out/contrary to industry standard" list:

• No spaces around the = sign for parameters with default values.

One for a later point in time once there is a sniff available which can enforce this:

• Only require the arrows in the innermost array to be aligned for nested arrays.

[jrfnl]

I've just opened PR #225 for yet another annoyance, but one which is serious enough that it decreases productivity. See the PR description for the reasoning.

[jrfnl]

Suggested by @fredden :

consider adding Squiz.Commenting.FunctionComment.ParamCommentNotCapital to phpcs.xml.dist

[fredden]

@jrfnl in #924 you mentioned some coding standard rules that you intend to change as part of the 4.0.0 release. That sounds related to this issue. Do you have a proposed phpcs.xml.dist that you could share here, please?

I think the idea is to eventually switch over to using PHPCSDevCS, but that we are looking for an incremental change on that journey for now.

[jrfnl]

@fredden For now I have bits and pieces in a few different branches - largely those things outlined above. It's one of the last changes I will apply to the branch as those commits would need updating every time something changes in the 3.x branch (and I rebase on top of it).