[Doc] Cover all errors of PEAR ClassDeclaration #856
Conversation
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.
There was a problem hiding this comment.
@braindawg Thank you for working on this and my apologies that it took me a while before I got to reviewing it.
FYI: I've looked at the documentation as a whole to review it, not just the additions you've made, so some feedback applies to pre-existing documentation.
When looking at the complete output, I notice the following things:
- There is a stray
sin the title of the docs-<documentation title="Class Declarations"> +<documentation title="Class Declaration">
- The rule description in the first
<standard>block reads a bit awkwardly. It also explicitly mentions "class", while the sniff (by now) acts on all named OO structures.
What do you think about changing it to something along the lines of the below ?-The opening brace of a class must be on the line after the definition by itself. +The opening brace of an OO structure must be on the line directly after the OO signature. The opening brace must be on a line by itself.
- The first and second code comparison basically demonstrate variations of the same rule. How about combining these into one code comparison block ? The "Invalid" side of that block could then have two code samples and the difference between those could be clarified via the class name.
Something like this:The "Invalid" description may need a tweak to allow for this.<![CDATA[ class BraceOnSignatureLine <em>{</em> } class BlankLineBetween <em></em> { } ]]>
- Along the same line, I think the last two code comparisons could be combined into one code comparison.
The "Invalid" description, again, may need a tweak to allow for this.
What do you think ?
Once approved & merged, we need to update the PSR2\Classes\ClassDeclaration documentation to include these cases, as well, since that sniff inherits this sniff but the docs do not currently cover the PEAR cases.
💯
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.
| @braindawg 👋🏻 Just checking in. Was the feedback I left clear enough ? |
Per advice from @jrfnl, clarified some language and combined some invalid examples to tighten the docs for this sniff.
Very clear! All suggestions should be taken into account with the latest commit. Sorry it took me so long to get back to this. |
2 participants
Description
While reviewing PR #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:
Once approved & merged, we need to update the
PSR2\Classes\ClassDeclarationdocumentation to include these cases, as well, since that sniff inherits this sniff but the docs do not currently cover the PEAR cases.Suggested changelog entry
Add documentation to cover all errors caught by the PEAR.Classes.ClassDeclaration sniff.
Related issues/external references
Requested in PR #844
Types of changes
PR checklist