-
-
Couldn't load subscription status.
- Fork 89
[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
[Docs] Add XML doc for Squiz.Classes.ClassDeclaration sniff #844
Conversation
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.
@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 ?
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. 👍
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 ?
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.
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).
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.
@braindawg I noticed the push to this PR - does this mean the PR is ready for review or are you still working on it ?
@braindawg I noticed the push to this PR - does this mean the PR is ready for review or are you still working on it ?
Still working - in fact, I've got to write up the PSR-2 version of this ClassDeclaration sniff doc based on the recent PEAR updates before I get back to the Squiz. I'll mark it ready once complete.
Still working - in fact, I've got to write up the PSR-2 version of this ClassDeclaration sniff doc based on the recent PEAR updates before I get back to the Squiz. I'll mark it ready once complete.
@braindawg Thanks for letting me know. I look forward to seeing the next step in this series.
@braindawg Just a reminder that this PR is still open and in draft ;-)
Description
Adds XML doc for the
Squiz\Classes\ClassDeclarationsniff.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
PR checklist