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

[Doc] Cover all errors of PEAR ClassDeclaration #856

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

Conversation

braindawg
Copy link
Contributor

@braindawg braindawg commented Mar 6, 2025

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:

  • OpenBraceWrongLine
  • OpenBraceNotAlone
  • SpaceBeforeBrace

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.

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

  • 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.

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.
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. There is a stray s in the title of the docs
    -<documentation title="Class Declarations">
    +<documentation title="Class Declaration">
  2. 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.
  3. 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:
            <![CDATA[
    class BraceOnSignatureLine <em>{</em>
    }
    
    class BlankLineBetween
    <em></em>
    {
    }
            ]]>
    The "Invalid" description may need a tweak to allow for this.
  4. 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.

}
]]>
</code>
<code title="Invalid: Opening brace is not alone on its line.">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<code title="Invalid: Opening brace is not alone on its line.">
<code title="Invalid: Opening brace is not on a line by itself.">

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