Improve error message for an invalid sniff code #344
Conversation
There was a problem hiding this comment.
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
--sniffsoption, 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.
|
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
force-pushed
the
feature/improve-sniff-code-error-message
branch
from
5fa0d0e to
1665e40
Compare
|
@jrfnl I think this is ready for another round of review. |
There was a problem hiding this comment.
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-widthis 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 to80to 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/--excludein green (same color as used in the help screen for the option name) - In the explaination line:
Standard.Category.Sniffin blue (same color as used in the help screen for the option name) ERRORin red.- For the error details: the invalid sniff code in blue ?
- In the suggestion line: have the suggestion in blue ? or maybe green ?
- In the explanation line ("The %s option only ..."):
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.
src/Config.php
Outdated
| * @return void | ||
| * @throws DeepExitException | ||
| */ | ||
| private function validateSniffCodes($sniffs, $argument) |
There was a problem hiding this comment.
| private function validateSniffCodes($sniffs, $argument) | |
| private function validateSniffCodes(array $sniffs, $argument) |
Alternatively, it could be considered to make this method handle the CLI input string.
What I mean by that is:
- Leave the
substr()call in the original logic; - Pass the CLI args value (minus the prefix) as a string;
- Move the
explode()to this method; - And then if everything is valid, let this method return the validated array of sniff codes.
src/Config.php
Outdated
| @@ -1658,4 +1644,62 @@ public function printConfigData($data) | |||
| }//end printConfigData() | |||
|
|
|||
|
|
|||
| /** | |||
| * Assert that all supplied sniff codes have the correct number of parts | |||
There was a problem hiding this comment.
| * Assert that all supplied sniff codes have the correct number of parts | |
| * Verify that all supplied sniff codes have the correct number of parts. |
Reason: assert is a PHP language construct with its own connotations and is not used here.
fredden
force-pushed
the
feature/improve-sniff-code-error-message
branch
from
df782e1 to
4d5b0c4
Compare
|
@jrfnl, I have made some changes based on your suggestions. Please can you take another look.
|
There was a problem hiding this comment.
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) ?
| @@ -1658,4 +1640,80 @@ public function printConfigData($data) | |||
| }//end printConfigData() | |||
|
|
|||
|
|
|||
| /** | |||
| * Parse supplied string into a list of sniff codes. | |||
There was a problem hiding this comment.
| * Parse supplied string into a list of sniff codes. | |
| * Parse supplied string into a list of validated sniff codes. |
| * @param string $argument The name of the argument which is being processed. | ||
| * | ||
| * @return string[] | ||
| * @throws DeepExitException |
There was a problem hiding this comment.
| * @throws DeepExitException | |
| * @throws DeepExitException When any of the provided codes are not valid as sniff codes. |
| if (preg_match('{[^A-Za-z0-9.]}', $sniff) === 1) { | ||
| $errors[] = 'Unsupported character detected: '.$sniff; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
What is this list of supported characters based on ?
Note: for arguments sake, I just ran some tests with a sniff called Generic.Пасха.💩💩 and PHPCS runs the sniff just fine...
| $sniffs = []; | ||
|
|
||
| $possibleSniffs = explode(',', $input); | ||
| $possibleSniffs = array_unique($possibleSniffs); |
There was a problem hiding this comment.
Any particular reason for the array_unique() call ?
Potentially relevant: array_unique() does a case-sensitive comparison when filtering.
* Too many parts: Generic.PHP.BacktickOperator.Found.TooMany
* Too many parts: generic.php.backtickoperator.found.toomany
| continue; | ||
| } | ||
|
|
||
| $partCount = substr_count($sniff, '.'); |
There was a problem hiding this comment.
Is substr_count() the correct function to use here ? Or should this be an explode() ?
Which also begs the question: should the validation be tolerant about a stray . at the end of a code ?
* Category codes are not supported: squiz.
* Standard codes are not supported: Squiz
There was a problem hiding this comment.
Missing: @covers \PHP_CodeSniffer\Config::parseSniffCodes
| $messageTemplate = 'ERROR: The specified sniff code "%s" is invalid'.PHP_EOL.PHP_EOL; | ||
| $messageTemplate = 'ERROR: The specified sniff code "%s" is invalid'.PHP_EOL; |
There was a problem hiding this comment.
This variable is now unused and can be removed.
| 'argument' => $argument, | ||
| 'value' => 'Standard', | ||
| 'message' => sprintf($messageTemplate, 'Standard'), | ||
| 'argument' => $argument, |
There was a problem hiding this comment.
As the data provider format has changed, the @return for the method needs to be updated.
| @@ -144,15 +189,50 @@ public static function dataValidSniffs() | |||
| $data = []; | |||
|
|
|||
| foreach ($arguments as $argument) { | |||
There was a problem hiding this comment.
As the data provider format has changed, the @return for the method needs to be updated.
| ]; | ||
| } |
There was a problem hiding this comment.
Should a test be added with a duplicate sniff code being passed ?
And maybe tests with variations in case ? (both for valid as well as invalid)
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
--sniffsor--excludecommand line arguments.Related issues/external references
Replaces / closes squizlabs/PHP_CodeSniffer#2646
Related to #319
Types of changes
PR checklist