Feedback on initial setup

[jrfnl]

Hi @Rarst, as discussed on Slack, please find some initial feedback below:

Note: all current feedback is just and only based on a quick look at the codebase. I haven't run any tests with the repo (yet).

composer.json:

• Why minimum PHP 7.2 ? Not needed for PHPCS (minimum PHP 5.4) and you're severely limiting the potential public which can use the sniff as people often don't want dev dependencies with a higher minimum PHP version as their own project (and Composer warns against doing that).
• Please remove the autoload section. This is known to cause issues with PHPCS.
• PHP_CodeSniffer should be a no-dev requirement, not a dev requirement. Otherwise users may still get an older PHPCS version if they also use other external standards which have a lower minimum PHPCS version.

.gitignore:

• Please ignore your tests.

Readme:

• Composer install: You may want to suggest for people to use require --dev as this sniff is normally not a production requirement for other packages.
• Standalone install:
  • missing instructions on installing PHPCS
  • I'd recommend telling people to set the installed_paths and use the ruleset by name. There are numerous bugs known for when standards are not installed.

Ruleset:

• You shouldn't need the <autoload> in the ruleset. PHPCS >= 3.1.0 should handle this fine as is. Though maybe not if the standard is not installed... hmm.... well, see my remark above in that case.
• Don't set properties for your own sniff in the ruleset. Set them as the default property value for the sniff instead (which you already have done).
  And you don't need to include the sniff in the ruleset, all sniffs in the Sniffs directory of a standard will automatically be included.
• Please add the XSD to the ruleset.
• You should also set the namespace you are using in the ruleset as it doesn't follow the base pattern expected by PHPCS, so you're breaking the autoloader.
  Have a look at WPCS to see an example.

Sniff code:

• See above my remark about the namespace. You can use this, but not without setting the namespace in the ruleset. You are currently breaking the build in autoloader from PHPCS.
• Minor nitpick: you are using non-standard names for the parameters received for process() making the cognitive load for other sniff devs higher (breaks the principle of least astonishment).
• $method: you are presuming a code style. Best practice sniffs should be code style agnostic.
  While not very common, this will break on:

  public function // some comment
      functionName() {}

• The PHPCS addError() function already has sprintf() build in, so no need to do it yourself. Just pass the replacements as an array in the fourth parameter.
• The errorcode should be descriptive and not contain characters which may break in XML. Using self::class is a bad idea as:
  1. It repeats the sniff name (useless).
  2. Is non-descriptive
  3. May break XML.
  4. Makes the sniff error code unnecessarily long.
     I'd suggest using a plain string like TooHigh.
     You could even consider setting two levels, like maxCognitiveComplexity and recommendedCognitiveComplexity, throw an error for the first, a warning for the second and have separate error code for each, like TooHigh and High

Analyzer code:

• $booleanOperators - you are missing three operators. Please consider using the PHPCS native Tokens::$booleanOperators array instead.
• $operatorChainBreaks - what about null coalesce ?
• For performance reasons, set the token arrays with the token constants as keys and use isset() instead of in_array().
• Again for performance reasons, do a quick check against which tokens you want to handle at the start of the main loop in computeFor...() and continue if not one of your targets.
  You are now also passing whitespace and comment tokens off for processing which is just plain inefficient.
• $nextToken in isIncrementingToken() is not code style agnostic.

Tests:

• These are definitely still using the wrong (copied) namespace.

Anyways, hope this helps.

[Rarst]

Why minimum PHP 7.2

I don't want to write lower than that in context of this project. Yes, adoption might depend on trying to support as low versions as possible, but this existing depends on me not being annoyed by it.

If someone needs lower versions support I'll happily consider getting paid for it. 😊

Please remove the autoload section.

Done.

PHP_CodeSniffer should be a no-dev requirement, not a dev requirement.

Eh, it's a bad fit for my preferred workflow. This works for me for the moment.

.gitignore: Please ignore your tests.

What? Why?

Readme

There is probably half a dozen way to go about it. I decided to just give couple fastest to get up and checking. Not intending to re-document PHPCS here.

You shouldn't need the in the ruleset.

Done, needed namespace to get this going.

Don't set properties for your own sniff in the ruleset. [...] And you don't need to include the sniff in the ruleset,

I know, did that while having a fight with it about getting sniff name right. Is this a problem particularly? It kinda serves as self-documenting example for someone reusing in downstream ruleset.

Please add the XSD to the ruleset.

Done?.. I pillaged from WPCS, PHPCS ruleset docs seem to make no mention of this...

See above my remark about the namespace.

Done. I started with it, but had a fight with whole thing getting the sniff name right.

you are using non-standard names for the parameters received for process()

Wasn't me. Done.

$method: you are presuming a code style.

Wasn't me. I get your point but don't know how to fix this right away.

The PHPCS addError() function already has sprintf() build in

Wasn't me. Done.

The errorcode should be descriptive and not contain characters which may break in XML.

Wasn't me. Done.

$booleanOperators - you are missing three operators.

The two are what spec covers. I will add more if I encounter that SonarLint (which I use as reference implementation) does anything with them.

$operatorChainBreaks - what about null coalesce ?

I am not sure. Spec says to favor constructs like these, so leaving it out for now.

For performance reasons, set the token arrays with the token constants as keys and use isset() instead of in_array()

Done.

Again for performance reasons, do a quick check against which tokens you want to handle at the start of the main loop

It already does this, just that there are different kinds of tokens we want to do different things with? It could probably be improved somewhat, but I don't see it right away.

$nextToken in isIncrementingToken() is not code style agnostic.

Again, don't know how to fix it right away...

Tests: These are definitely still using the wrong (copied) namespace.

As in adding namespace to ruleset (done as above) or something else?

Thanks a lot for extensive feedback! :)

[Rarst]

Ah, found the leftover namespace. :)

[jrfnl]

.gitignore: Please ignore your tests.

What? Why?

Sorry, I meant export-ignore the tests in the .gitattributes file.