Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest sniff code if invalid #2646

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tdgroot
Copy link

@tdgroot tdgroot commented Oct 11, 2019

This change is a quality of life improvement. I wanted to exclude a sniff, namely Generic.WhiteSpace.ScopeIndent.Incorrect. This sniff code came from the phpcs report that showed an error I wanted to ignore. When I entered this, phpcs told me:

ERROR: The specified sniff code "Generic.WhiteSpace.ScopeIndent.Incorrect" is invalid

Run "phpcs --help" for usage information

So why is the sniff code invalid? After digging in the source code, I found that a sniff code may consist of only 2 periods.

With this change, the output will look like the following:

ERROR: The specified sniff code "Generic.WhiteSpace.ScopeIndent.Incorrect" is invalid

Perhaps try following sniff code: Generic.WhiteSpace.ScopeIndent

Run "phpcs --help" for usage information

@gsherwood
Copy link
Member

Thanks for the PR. I intend on allowing Standard, Category, Sniff, and message codes on the CLI at some point in the future so that the feature set lines up with how rulesets work, but this is a nice message to have in the meantime.

The only issue I have with the PR is that it only works for 4-part codes and so the message is misleading when anything else is used.

For example, when using a standard or category name (as you can in a ruleset):

$ phpcs temp.php --exclude=Generic
ERROR: The specified sniff code "Generic" is invalid

Perhaps try following sniff code: Generic

Run "phpcs --help" for usage information

$ phpcs temp.php --exclude=Generic.WhiteSpace
ERROR: The specified sniff code "Generic.WhiteSpace" is invalid

Perhaps try following sniff code: Generic.WhiteSpace

Run "phpcs --help" for usage information

A good change might be to only make the suggestion of the sniff code when there are exactly 4 parts to the specified code. If a user specifies 4 parts, they have probably used a message code instead. But ff they specify less than 3 or more than 4, they probably need a more specific error message than current "The specified sniff code is invalid" message.

So maybe for a 4-part code, the message could look something like:

ERROR: The specified sniff code "Generic.WhiteSpace.ScopeIndent.Incorrect" is invalid

Sniff codes are in the form "Standard.Category.Sniff"

Perhaps try "Generic.WhiteSpace.ScopeIndent" instead

Run "phpcs --help" for usage information

And for anything else, the message could just have:

ERROR: The specified sniff code "Generic.WhiteSpace" is invalid

Sniff codes are in the form "Standard.Category.Sniff"

Run "phpcs --help" for usage information

This finally gives the error some actual information about what is expected, but also keeps that suggestion for the case when a message code is used.

Of course, we could extend it even further to print more information like:

ERROR: The specified sniff code "Generic" is invalid

This appears to be a Standard code, but the exclude option only supports sniff codes. Sniff codes are in the form "Standard.Category.Sniff".

and

ERROR: The specified sniff code "Generic.WhiteSpace" is invalid

This appears to be a Category code, but the exclude option only supports sniff codes. Sniff codes are in the form "Standard.Category.Sniff".

and

ERROR: The specified sniff code "Generic.WhiteSpace.ScopeIndent.Incorrect" is invalid

This appears to be a Message code, but the exclude option only supports sniff codes. Sniff codes are in the form "Standard.Category.Sniff".

Perhaps try "Generic.WhiteSpace.ScopeIndent" instead

What do you think about those suggestions?

@jrfnl
Copy link
Contributor

jrfnl commented Jul 13, 2023

While I like the principle of this change, the PR was never updated after the feedback by @gsherwood, so I'm marking it as a candidate for closure now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants