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

docs and output are unclear on what counts as a sniff code #319

Closed
3 tasks done
joachim-n opened this issue Jan 30, 2024 · 17 comments · Fixed by #328
Closed
3 tasks done

docs and output are unclear on what counts as a sniff code #319

joachim-n opened this issue Jan 30, 2024 · 17 comments · Fixed by #328

Comments

@joachim-n
Copy link
Contributor

joachim-n commented Jan 30, 2024

Describe the bug

Using the -s option shows sniffs in the report, like this:

 17 | ERROR | [x] Line indented incorrectly; expected at least 8 spaces, found 4 (PEAR.WhiteSpace.ScopeIndent.Incorrect)

However, passing that sniff name to the --sniffs or --exclude flag produces this error:

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

It turns out that that's because this isn't a full sniff. Passing PEAR.WhiteSpace.ScopeIndent DOES work.

Expected behavior

The docs and the behaviour are contradicting each other, and misleading the user. The output for --help says:

-s Show sniff codes in all reports

Therefore the user assumes that the output added by this option consists of sniff codes, and that, for example, 'PEAR.WhiteSpace.ScopeIndent.Incorrect' is one such sniff code.

But help for --sniffs and --excludes options says:

 <sniffs>       A comma separated list of sniff codes to include or exclude from checking
                (all sniffs must be part of the specified standard)

This also says 'sniff codes' but it disagrees with -s on what a sniff code actually consists of!

Versions (please complete the following information)

Operating System MacOS 10.15]
PHP version 8.3
PHP_CodeSniffer version 3.8.1
Standard -- any --
Install type Composer local

Additional context

Add any other context about the problem here.

Please confirm:

  • I have searched the issue list and am not opening a duplicate issue.
  • I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
  • I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@fredden
Copy link
Member

fredden commented Jan 30, 2024

See also squizlabs/PHP_CodeSniffer#2646

@jrfnl
Copy link
Member

jrfnl commented Jan 30, 2024

-s Show sniff codes in all reports

@joachim-n Might be good to update that to "error codes". Want to submit a PR for this ?

For reference:

  • Sniff code - three parts PEAR.WhiteSpace.ScopeIndent
  • Error code - four parts PEAR.WhiteSpace.ScopeIndent.Incorrect

Build up like so: StandardName.CategoryName.SniffName.ErrorCode

From the command line, you can (currently) only include/exclude sniff codes.

In a ruleset, you can include/exclude based on any variation:
202212 LeicesterUni - PHPCS4QA - errorcodes

(Slide source: https://speakerdeck.com/jrf/managing-code-quality-and-code-consistency?slide=22 / https://speakerdeck.com/jrf/dont-work-for-phpcs-make-phpcs-work-for-you-4?slide=29 )

@joachim-n
Copy link
Contributor Author

I've made a PR with the fix, but I can't see any mention of the term 'error code' in the docs at https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Advanced-Usage.

@joachim-n
Copy link
Contributor Author

Could the PR at squizlabs/PHP_CodeSniffer#2646 be brought into this repo?

@jrfnl
Copy link
Member

jrfnl commented Feb 5, 2024

Could the PR at squizlabs/PHP_CodeSniffer#2646 be brought into this repo?

@joachim-n You could ask the OP to pull it here, but considering that the OP never responded to the PR review and never made the requested changes and those changes would still need to be made, I'm not sure you'll get much of a response.

@jrfnl
Copy link
Member

jrfnl commented Feb 5, 2024

I've made a PR with the fix, but I can't see any mention of the term 'error code' in the docs at https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Advanced-Usage.

@joachim-n If you have any suggestions on where updates are needed, please leave them here and I'll update the wiki.

Other than that, I'd very much like to (eventually) move the docs from the wiki to a dedicated GH Pages based website. This would allow for them to be updated via pull requests and make it more managable in general.

Maybe we should start collecting topics on which articles should be written for the website by opening issues in the website repo. Do you want to open an issue for this ?

@joachim-n
Copy link
Contributor Author

I would probably break up this paragraph on the home page:

A coding standard in PHP_CodeSniffer is a collection of sniff files. Each sniff file checks one part of the coding standard only. Multiple coding standards can be used within PHP_CodeSniffer so that the one installation can be used across multiple projects. The default coding standard used by PHP_CodeSniffer is the PEAR coding standard.

into this:

A coding standard in PHP_CodeSniffer is a collection of sniff files. Each sniff file checks one part of the coding standard only. Each sniff is idenfied by a three-part name derived from the sniff file, for example 'Some.Example.Here'. Sniffs can (?) have multiple error codes, identified by a four-part name, for example 'Some.Example.Here.Specific'.

Multiple coding standards can be used within PHP_CodeSniffer so that the one installation can be used across multiple projects. The default coding standard used by PHP_CodeSniffer is the PEAR coding standard.

I've not seen docs on the -s option -- that's where I'd also add something to explain how to pass those back into --sniffs or --exclude.

@jrfnl
Copy link
Member

jrfnl commented Feb 5, 2024

@joachim-n Looking at the wiki now. Your suggestion feels a little too detailed for the homepage, though I agree that expanding the text a little would be good.

What about something like this:

A coding standard in PHP_CodeSniffer is a collection of sniff files. Each sniff file checks one part of the coding standard only. Each sniff can yield multiple error codes, a different one for each aspect of the code which was checked and found non-compliant.

Multiple coding standards can be used within PHP_CodeSniffer so that the one installation can be used across multiple projects. The default coding standard used by PHP_CodeSniffer is the PEAR coding standard.

I've not seen docs on the -s option -- that's where I'd also add something to explain how to pass those back into --sniffs or --exclude.

There is a section on the use of the --sniffs and --exclude options on the Advanced Usage page.

@joachim-n
Copy link
Contributor Author

+1 to your suggestion -- I agree that getting into the details of how the names are formed was rather detailed for the home page, but wasn't sure where else to put it.

There is a section on the use of the --sniffs and --exclude options on the Advanced Usage page.

Yup, but the -s option for showing error code names in the output doesn't seem to be mentioned in the wiki docs. That's where I'd put something about the difference between sniff codes and error codes.

@jrfnl
Copy link
Member

jrfnl commented Feb 5, 2024

Yup, but the -s option for showing error code names in the output doesn't seem to be mentioned in the wiki docs. That's where I'd put something about the difference between sniff codes and error codes.

@joachim-n The -s option is mentioned on the Reporting (Full/Summary report + Source report)
It is also mentioned a couple of times in the comments for examples in the Annotated Ruleset.

@joachim-n
Copy link
Contributor Author

Ah! I hadn't looked on that page :/

The docs there say "To include source codes in the report, use the -s command line argument" - should that be changed to 'error codes' since that's the term we've just used in the command help?

I would suggest adding a paragraph after the screenshot to explain how to pass the error codes to the --sniffs and --exclude options.

@jrfnl
Copy link
Member

jrfnl commented Feb 6, 2024

@joachim-n Not sure what you mean with "the screenshot" ?

@jrfnl
Copy link
Member

jrfnl commented Feb 6, 2024

@joachim-n Been playing around a bit with this, what about this:

image
image
image

(sorry for the screenshots, but the wiki doesn't allow for branches and linking to them)

@joachim-n
Copy link
Contributor Author

That looks great, thanks!

(By screenshot I meant the sample of terminal output showing the report with the error codes.)

@jrfnl
Copy link
Member

jrfnl commented Feb 6, 2024

@joachim-n Thanks. Pushed!

@jrfnl jrfnl added this to the 3.9.0 milestone Feb 15, 2024
@joachim-n
Copy link
Contributor Author

You could ask the OP to pull it here, but considering that the OP never responded to the PR review and never made the requested changes and those changes would still need to be made, I'm not sure you'll get much of a response.

Do the terms of how contributions are licensed allow someone else to push that branch onto the new repository and make a MR here?

@jrfnl
Copy link
Member

jrfnl commented Feb 16, 2024

@joachim-n You'd have to be careful to make sure the original commit(s) are retained as they were as otherwise, we'd be talking copyright violation.

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

Successfully merging a pull request may close this issue.

3 participants