Settle on coding convention for WPCS snfifs

[westonruter]

See a446a38#commitcomment-8129539.

But in reality this is the least of the problems in the coding style of the WPCS sniffs. It's currently a mismash of the conventions outlined in the PHPCS coding standards and the WPCS coding standards. There needs to be a comprehensive sweep done to fix them up (after all PRs are merged).

[GaryJones]

@westonruter Do you have any preference for which CS we use?

[westonruter]

WPCS will be problematic due to the camelCase methods of PHPCS. So I think we'll have to use the PHPCS standards.

[GaryJones]

That makes sense. It will also make it consistent when looking at Generic / Squiz and WP sniffs.

[JDGrimes]

WPCS will be problematic due to the camelCase methods of PHPCS. So I think we'll have to use the PHPCS standards.

I'd really much prefer if we use the WordPress standards, at least as far as whitespace goes. An added benefit of using WPCS itself, is that with an IDE like phpStorm you can see what is failing/succeeding when editing the .inc test files before you even run the tests.

It will also make it consistent when looking at Generic / Squiz and WP sniffs.

Honestly, I don't think that'd be such a good thing 😈. Seriously though, I think parity with WordPress's coding standards would be much easier on contributors, since that is what they'll be used to and probably expect.

[GaryJones]

Just like PHPCS has a PHPCS Standard that itself follows, we can do the same with a WPCS standard, where we can mostly use the WordPress standard we're familiar with, but allow for camel case method names and other things needed as we extend PHPCS.

[westonruter]

+1

[JDGrimes]

We already allow for camelCase in subclasses anyway (#307).

So, are we saying, "WordPress-Core is our standard going forward", or do we want to clean up all of the sniffs now?

[westonruter]

Either way, but for the cleanup of all existing sniffs we just wouldn't want to make any headaches for any open pull requests.

[GaryJones]

To be able to check it properly, I think all the sniffs have to be cleaned up, but minimising the effect on open PRs. If we're waiting on improved PRs, give PR authors a nudge to get it improved in the next couple of weeks. If the PRs is ready, get it merged into 0.4.0, then let's make 0.5.0 the version where we meet our own standards.

FWIW, we only have six open PRs - three for @JDGrimes , one for @westonruter and two for @shadyvb so this is pretty much an internal nudge anyway.

[JDGrimes]

I think it will be much easier to update the sniffs to match the coding standards once we've implemented automatic error fixing for more of them (#388). Let's wait until then.

[jrfnl]

There seems to be consensus about this - so what about setting up the standard and adding it to the travis build script ?
This would help prevent new coding style issues being introduced by new sniffs and the like. Nudging because of #578 which will introduce a lot of new sniffs.

[westonruter]

@jrfnl I agree. Also, we can use wp-dev-lib's CHECK_SCOPE=patches to limit Travis to only report PHPCS errors on the actual lines of code being introduced. This will help us transition to the PHPCS standard of our chosing. I think we should go with WordPress-Core as the PHPCS standard to start with.

[jrfnl]

we can use wp-dev-lib's CHECK_SCOPE=patches to limit Travis to only report PHPCS errors on the actual lines of code being introduced. This will help us transition to the PHPCS standard of our chosing. I think we should go with WordPress-Core as the PHPCS standard to start with.

@westonruter Love the idea, just not all that sure how to implement it. Shell is definitely not my strong point.
Will have a play with it, but pointers appreciated.

[jrfnl]

Result of an initial run isn't too bad if you take spaces vs tabs out of the equation.

    <rule ref="WordPress-Core">
        <exclude name="Generic.Files.LowercasedFilename" />
        <exclude name="WordPress.NamingConventions.ValidVariableName" />
    </rule>

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
-------------------------------------------------------------------------------------
    SOURCE                                                                      COUNT
-------------------------------------------------------------------------------------
[x] Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed                           1399
[x] WordPress.Arrays.ArrayKeySpacingRestrictions.NoSpacesAroundArrayKeys        183
[x] PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket                167
[x] PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket                  166
[x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterOpenParenthesis    154
[x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeCloseParenthesis  147
[ ] WordPress.PHP.YodaConditions.NotYoda                                        141
[x] Generic.Functions.OpeningFunctionBraceKernighanRitchie.BraceOnNewLine       103
[x] WordPress.WhiteSpace.ControlStructureSpacing.OpenBraceNotSameLine           103
[x] Generic.WhiteSpace.ScopeIndent.IncorrectExact                               25
[x] WordPress.WhiteSpace.ControlStructureSpacing.BlankLineAfterEnd              23
[x] WordPress.Arrays.ArrayDeclaration.NoSpaceAfterOpenParenthesis               18
[x] Squiz.WhiteSpace.SuperfluousWhitespace.EndLine                              18
[x] Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterOpenHint     11
[x] Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose       11
[x] Squiz.Strings.ConcatenationSpacing.PaddingFound                             10
[x] Generic.WhiteSpace.ScopeIndent.Incorrect                                    9
[x] Squiz.ControlStructures.ControlSignature.SpaceAfterCloseBrace               8
[x] WordPress.Arrays.ArrayDeclaration.SpaceInEmptyArray                         8
[x] Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines                           6
[x] Generic.ControlStructures.InlineControlStructure.NotAllowed                 5
[x] PSR2.ControlStructures.ElseIfDeclaration.NotAllowed                         4
[x] PEAR.Functions.FunctionCallSignature.Indent                                 4
[x] WordPress.Arrays.ArrayDeclaration.NoComma                                   3
[x] Squiz.ControlStructures.ControlSignature.SpaceAfterKeyword                  2
[x] WordPress.Arrays.ArrayDeclaration.NoCommaAfterLast                          2
[x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeOpenParenthesis   1
[x] WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterStructureOpen      1
-------------------------------------------------------------------------------------
A TOTAL OF 2732 SNIFF VIOLATIONS WERE FOUND IN 28 SOURCES
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 27 MARKED SOURCES AUTOMATICALLY (2591 VIOLATIONS IN TOTAL)
-------------------------------------------------------------------------------------

[jrfnl]

Just thinking: for me to try and figure out how to get the CHECK_SCOPE working will probably take just as much time as just taking the nuclear option and fixing the code base.

Do I have support for doing that (fixing the code style of the whole code base in one go) ?

[westonruter]

@jrfnl I've implemented this in #587. Once merged, it should then cause Travis to run a build on develop and identify the code differences in develop that are violating the WordPress-Core standard, namely:

WordPress/Sniffs/Arrays/ArrayAssignmentRestrictionsSniff.php:169:25: error - Variable "stackPtr" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/Arrays/ArrayAssignmentRestrictionsSniff.php:170:25: error - Variable "groupName" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:52:51: error - Variable "phpcsFile" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:52:63: error - Variable "stackPtr" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:54:22: error - Variable "phpcsFile" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:56:57: error - Variable "stackPtr" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:60:26: error - Variable "phpcsFile" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/VIP/SlowDBQuerySniff.php:60:38: error - Variable "stackPtr" is not in valid snake_case format (WordPress.NamingConventions.ValidVariableName.NotSnakeCase)
WordPress/Sniffs/WP/I18nSniff.php:1:1: error - Filename "I18nSniff.php" doesn't match the expected filename "i18nsniff.php" (Generic.Files.LowercasedFilename.NotFound)
WordPress/Tests/VIP/SlowDBQueryUnitTest.php:45:1: error - Tabs must be used to indent lines; spaces are not allowed (Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed)
WordPress/Tests/WP/I18nUnitTest.php:1:1: error - Filename "I18nUnitTest.php" doesn't match the expected filename "i18nunittest.php" (Generic.Files.LowercasedFilename.NotFound)

(Above found via DEV_LIB_ONLY=phpcs DIFF_BASE=master ./dev-lib/pre-commit.)

[westonruter]

We could try using phpcbf across the entire codebase, but this would result in a huge diff and I'd rather we clean it up as we go, similar to how this is done in WordPress Core itself.

[JDGrimes]

We could try using phpcbf across the entire codebase, but this would result in a huge diff and I'd rather we clean it up as we go, similar to how this is done in WordPress Core itself.

Personally, I'd prefer that we do that. I know that it can make history harder to traverse, but I find it more confusing when a project is inconsistent. But I'll leave it up to y'all. I'll be happy either way. 😄

[jrfnl]

My offer to do a nuclear clean up still stands. In that case we wouldn't need the additional dependency on the submodule.

Either way is fine with me, but would be great if we could get to a decision some time within the next few days as it will impact the Theme Check fork/future PR if we want to create a semi-clean history on that end.

[westonruter]

I think it would be a benefit to use wp-dev-lib anyway, but regardless, if you want to do a nuclear clean up, go for it. Doing phpcbf --standard=WordPress-Core --extensions=php . would get the first pass. It will probably need a manual look-over for each PHP file (at least to smoke test) to just make sure it did the right thing.

[JDGrimes]

We should merge all most (all ready) pull requests first though, as there'll likely be conflicts otherwise.

[JDGrimes]

Scratch that. Looks like we don't have any open PRs that would cause conflicts. Go for it.

[jrfnl]

Going for it ;-)

Oh and I already made sure the open PR for the alternative open tags will comply with the standard.

[jrfnl]

Doing phpcbf --standard=WordPress-Core --extensions=php . would get the first pass

Unfortunately that doesn't work on Windows, but not to worry, got my own tricks, not my first rodeo.

[jrfnl]

While I'm at it - shall I remove unused (commented out) code as well ?

[jrfnl]

😀 starting to be able to tell who's worked on which file based on code style ;-)

1/3 down, 2/3 to go

[jrfnl]

All done. Expect PR later today.

[jrfnl]

PR submitted - #590

[jrfnl]

I think it would be a benefit to use wp-dev-lib anyway

@westonruter Curious to hear what benefits this will bring other than the PR code style diff (which shouldn't be needed anymore now).
If it'd still be useful - rebase your PR after the code style fix one has been merged ? No reason not to have the best of both worlds 😎