Improve error message for an invalid sniff code

[fredden]

Description

This is an implementation of the suggestions made by @gsherwood in squizlabs/PHP_CodeSniffer#2646 (comment)
I have intentionally not looked at the code changes initially suggested in squizlabs/PHP_CodeSniffer#2646 (to avoid any copyright woes).

Suggested changelog entry

Improved error message when invalid sniff codes are supplied to --sniffs or --exclude command line arguments.

Related issues/external references

Replaces / closes squizlabs/PHP_CodeSniffer#2646
Related to #319

Types of changes

• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

Thanks for this PR @fredden.

Unfortunately, there are no pre-existing tests covering the changes, so new tests will need to be written to cover this change - as the line you removed from the "PR checklist" pointed out... (that checklist item is there for a reason... I've added it back now)

Some observations when I look at the behaviour around this error message:

• If multiple erroneous names are passed via the --sniffs option, only the first error is displayed, so the user fixes that one, runs PHPCS again and will then be confronted with the next error.
  I believe this should be made more user friendly.
• If the list of sniffs contains a stray comma, like --sniffs=A.B.C,,D.E.F, this will be reported as "The specified sniff code "" is invalid".
  I believe we can filter out/ignore empty sniff codes altogether without bothering the end-user with this.

As this PR introduces a dedicated method to validate the sniff codes (👍🏻), I think we should take this opportunity to make the fix comprehensive.

Other than that, I find the messaging a bit "wordy", so I wonder whether it can be made more succinct.

[fredden]

I have discussed this with @jrfnl. I intend to add tests to cover the existing behaviour in a separate pull request. I'll mark this as a draft until that change is complete. I'll come back to this when that change is done and make any necessary changes here at that time.

[fredden]

@jrfnl I think this is ready for another round of review.

[jrfnl]

Hi @fredden, great step in the right direction!

This iteration is getting closer to what I had in mind, but I do still have some nitpicks about the message order, message text and the look and feel.

These are screenshots I took with the new output:

When I look at these, I think the readability can be improved further.

These would be my suggestions to improve this (feel free to deviate/let's discuss this further, nothing is set in stone):

• Move the "The %s option only supports sniff codes. Sniff codes are in the form "Standard.Category.Sniff"" up to be at the start, so it is straight away clear what these messages are about.
• "ERROR: The specified sniff code "squiz" is invalid
  This appears to be a Standard code."
  What about folding this into a single sentence ?
  Something like "ERROR: the specified Standard code "squiz" is not supported" ?
  Also note, I'd recommend using "not supported" instead of "invalid". There has been talk in the past about adding support for more than just sniff codes. While this is not on the roadmap for the immediate future, let's leave the option open that it will be added to the roadmap at some point.
• Or maybe listing the individual errors instead of having sentences might be better yet ?
  "The following sniff codes provided are not supported:
  • squiz (Standard code)
  • pear.commenting (Category code)
  • ... etc"
• And if the above suggestion would be applied, the "ERROR" prefix could maybe be moved to the first sentence ("The %s option only...") ?
• I have a feeling that a potentially set report-width is not taken into account for these error messages.
  I'm not saying it should be (as it can get complicated), but if not, maybe limit the line length for error message to 80 to be on the safe side ?
  (the suggestion in bullet three would help with that)
• Use proper punctuation in all messages.
  I realize that pre-existing messages don't always do this, but let's do it right for any new messages (and maybe open an issue to review the grammar and punctuation of existing messages ?).
• Have an extra new line at the end to separate the suggestion sentence from the generic "Run "phpcs --help" for usage information" message ?
• If the user has color support enabled, maybe use some colors in the message(s) ?
  Suggestions:
  • In the explanation line ("The %s option only ..."): --sniffs/--exclude in green (same color as used in the help screen for the option name)
  • In the explaination line: Standard.Category.Sniff in blue (same color as used in the help screen for the option name)
  • ERROR in red.
  • For the error details: the invalid sniff code in blue ?
  • In the suggestion line: have the suggestion in blue ? or maybe green ?

Other than that, my previous remark about stray comma's still remains:

If the list of sniffs contains a stray comma, like --sniffs=A.B.C,,D.E.F, this will be reported as "The specified sniff code "" is invalid".
I believe we can filter out/ignore empty sniff codes altogether without bothering the end-user with this.

Note: I've not reviewed the test changes in detail yet as with the above feedback I suspect those will still be changed for the next iteration anyway.

[fredden]

@jrfnl, I have made some changes based on your suggestions. Please can you take another look.

[jrfnl]

He @fredden, thank you for the update to the PR. I like where this is going.

Thank you also for addressing the stray comma issue, though it does appear to have a side-effect:

phpcs -ps . --sniffs=
phpcs -ps . --sniffs=,

In both of the above cases, the invalid --sniffs (or --exclude) directive is now completely ignored, while previously this would throw an error.

Is this intentional ?

I've also done some more (stress) testing.

phpcs -psl . --sniffs=squiz,pear.commenting,,Generic.Arrays.DisallowShortArray,generic.arrays.disallowshortarray.found,Generic.PHP.BacktickOperator.Found.TooMany,generic.php.backtickoperator.found.toomany,psr12.operators.operatorspacing,,,squiz.commenting,psr1,generic.php.lowercasetype.TypeCastFound,squiz.,Squiz,

The output of this looks like this:

Feels like this still leaves some room for further readability improvements.

Other than that, AFAICS, the below two points from my previous review have not been addressed yet, either by making an argument against the suggestion or by applying it.

• I have a feeling that a potentially set report-width is not taken into account for these error messages.
  I'm not saying it should be (as it can get complicated), but if not, maybe limit the line length for error message to 80 to be on the safe side ?
• If the user has color support enabled, maybe use some colors in the message(s) ?

[fredden]

phpcs -ps . --sniffs=
phpcs -ps . --sniffs=,

In both of the above cases, the invalid --sniffs (or --exclude) directive is now completely ignored, while previously this would throw an error.
Is this intentional ?

No. I've made a code change to address this.

I have a feeling that a potentially set report-width is not taken into account for these error messages.
I'm not saying it should be (as it can get complicated), but if not, maybe limit the line length for error message to 80 to be on the safe side ?

Respecting the report-width parameter sounds like a good feature we can add later.

If the user has color support enabled, maybe use some colors in the message(s) ?

Adding some colour sounds like a good feature we can add later.