Skip to content

[Docs] Add XML doc for Squiz.Classes.ClassDeclaration sniff #844

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

braindawg
Copy link
Contributor

Description

Adds XML doc for the Squiz\Classes\ClassDeclaration sniff.

This sniff inherits from a PSR2 sniff which inherits from PEAR, so I'm not sure how much to document here. For now I've documented only the checks added by the Squiz version. This seems consistent with the documentation of the PSR2 sniff, which only covers cases not already covered by PEAR.

Suggested changelog entry

Add XML doc for Squiz.Classes.ClassDeclaration sniff

Related issues/external references

Partial fix for #148

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • 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.

This sniff inherits from a PSR2 sniff which inherits from PEAR, so I'm
not sure how much to document here. For now I've documented only the
checks added by the Squiz version.
@jrfnl
Copy link
Member

jrfnl commented Feb 25, 2025

@braindawg Thanks for taking on this sniff.

This sniff inherits from a PSR2 sniff which inherits from PEAR, so I'm not sure how much to document here. For now I've documented only the checks added by the Squiz version. This seems consistent with the documentation of the PSR2 sniff, which only covers cases not already covered by PEAR.

Hmm... I don't actually think that's correct as the documentation generation doesn't take classes extending each other into account, so wouldn't display the PSR2/PEAR documentation to make the documentation complete.

And looking at the PEAR/PSR2 docs for the ClassDeclaration sniffs, the PEAR docs don't look complete (don't properly cover all error codes) and the PSR2 docs seem to have all rules in one <standard> block, instead of split out into multiple standard blocks and only contains one code sample, while there should be multiple.

It might be an idea to approach this one more holistically and improve the docs of all three of the sniffs involved.

Would you want to take that on ?

@braindawg
Copy link
Contributor Author

Agreed, I can do that. Will add separate issues for the other two sniffs, and separate PRs for all 3 (including this) unless you direct me otherwise. 👍

@jrfnl
Copy link
Member

jrfnl commented Feb 26, 2025

Agreed, I can do that. Will add separate issues for the other two sniffs, and separate PRs for all 3 (including this) unless you direct me otherwise. 👍

@braindawg Separate PRs for the other two sounds good, though I suppose we could move this PR to draft for the time being and it can then be updated later ?

I suspect it will be most straight-forward to start with the PEAR one (lowest in the chain), then parts of the docs written for PEAR can be re-used for the PSR2 sniff and then same again after that for this sniff. Does that make sense ?

@braindawg braindawg marked this pull request as draft February 27, 2025 16:05
braindawg added a commit to braindawg/PHP_CodeSniffer that referenced this pull request Mar 6, 2025
While reviewing PR PHPCSStandards#844 for the Squiz version of this sniff, jrfnl
discovered that the PEAR sniff's documentation did not cover all error
conditions.

This adds code examples for all missing errors that should be caught by
this sniff.
@braindawg
Copy link
Contributor Author

braindawg commented Mar 11, 2025

Just to clarify - in the case of an inherited sniff, is the preferred method for pulling in docs from e.g. PEAR to PSR2 a copy/paste (or more nuanced merge) into the individual PSR2 sniff's XML? Or should I include the PEAR sniff in the PSR2 ruleset.xml?

With the second method, we avoid code duplication, but we get naming collisions, which has already happened with the PSR1 ruleset being ref'd in to PSR2 and PSR12 (there are currently duplicate "Class Declaration" titles in each generated standard doc as a result).

@jrfnl
Copy link
Member

jrfnl commented Mar 22, 2025

Just to clarify - in the case of an inherited sniff, is the preferred method for pulling in docs from e.g. PEAR to PSR2 a copy/paste (or more nuanced merge) into the individual PSR2 sniff's XML?

It should be a (selective) copy/paste for only those errors/warnings which are inherited.
This needs to be done carefully as some parent methods may be overloaded in the child class, which means that some errors/warnings from the parent sniff may not apply to the child sniff.

Or should I include the PEAR sniff in the PSR2 ruleset.xml?

Adding the PEAR sniff to the PSR2 ruleset.xml file is not an option. This would lead to duplicate errors when people would run the PSR2 ruleset (as both sniffs would run).
And in the case of sniffs where methods are overloaded and not all errors/warnings from the parent are in use by the child, it could lead to errors/warnings being shown from the parent sniff which do not apply for the standard containing the child sniff.

With the second method, we avoid code duplication, but we get naming collisions, which has already happened with the PSR1 ruleset being ref'd in to PSR2 and PSR12 (there are currently duplicate "Class Declaration" titles in each generated standard doc as a result).

Along the same lines and as I mentioned in the other PR:

I've also been thinking about whether we could let the documentation for sniff classes which extend other sniff classes, "inherit" the documentation of the parent sniff, but I don't think we can, as methods can be overloaded in the child sniff class, so not all documentation which is valid for the parent sniff, may apply to the child sniff.

So, unfortunately, some documentation code duplication is unavoidable as things are at the moment.

Bright ideas on how to improve this situation - possibly with a change in the documentation system - are very welcome. If you have any, please open an issue about this. However, we cannot code for a hypothetical future, so until that time, we should not block progress on improving the sniff documentation.

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.

2 participants