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

Generic/EmptyPHPStatement: add XML documentation #166

Merged

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Dec 14, 2023

Description

This PR adds documentation for the Generic/EmptyPHPStatement sniff.

I added one <standard> block per warning generated by this sniff. I considered adding another <standard> block to the beginning of the file summarizing what this sniff does. Something like:

<documentation title="Empty PHP Statement">
    <standard>
    <![CDATA[
    Empty PHP tags and superfluous semi-colons are not allowed.
    ]]>
    </standard>
    (...)
</documentation>

It does duplicate the content of the other <standard> blocks, but it provides a nice summary at the top of the documentation. Without the summary, there is a chance that someone skimming through the documentation might assume that this sniff is only about empty PHP tags and doesn't cover superfluous semi-colons.

I ended up not including the "summary" <standard> most of the other sniffs that contain multiple <standard> blocks don't contain a summary at the top. The only exception that I found is ArrayDeclarationStandard.xml. Any thoughts on this?

Suggested changelog entry

Add documentation for the Generic.CodeAnalysis.EmptyPHPStatement sniff

Related issues/external references

Part of #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.

@jrfnl jrfnl changed the title Generic/EmptyPHPStatement: add documentation Generic/EmptyPHPStatement: add XML documentation Dec 15, 2023
@rodrigoprimo rodrigoprimo force-pushed the empty-php-statement-documentation branch from 60eeaca to caf08e7 Compare December 20, 2023 12:21
@rodrigoprimo
Copy link
Contributor Author

One of the GitHub actions failed when trying to download PHPCS phar and it seems I don't have permission to run it again: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7275269656/job/19822876012?pr=166. I imagine it was due to a temporary problem that is not related to the changes in this PR.

@jrfnl
Copy link
Member

jrfnl commented Jan 3, 2024

One of the GitHub actions failed when trying to download PHPCS phar

This is a known issue and shouldn't happen anymore (for now) - see #204

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.

Thanks for this PR @rodrigoprimo .

I considered adding another <standard> block to the beginning of the file summarizing what this sniff does.
...
It does duplicate the content of the other <standard> blocks, but it provides a nice summary at the top of the documentation.
...
Any thoughts on this?

I'm not against adding a summary block at the start when it has value, but I don't see it having value when it just repeats what's already in the other blocks.

So:
Empty PHP tags and superfluous semi-colons are not allowed.
✔️ Empty PHP statements are not allowed (=> the empty PHP tag sets and superfluous semi-colons are symptoms, empty PHP statements is the actual problem the sniff addresses.)

@rodrigoprimo
Copy link
Contributor Author

I'm not against adding a summary block at the start when it has value, but I don't see it having value when it just repeats what's already in the other blocks.

So:
❌ Empty PHP tags and superfluous semi-colons are not allowed.
✔️ Empty PHP statements are not allowed (=> the empty PHP tag sets and superfluous semi-colons are symptoms, empty PHP > statements is the actual problem the sniff addresses.)

Thanks for your input. In the end, I decided not to include a summary description at the top. I'm not super sure about and after trying it now it felt a little bit weird and repetitive since there is no clear separation between summary description and the description for the first <code_comparison> block.

This PR is ready for a final review.

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.

LGTM! Thanks @rodrigoprimo !

@jrfnl jrfnl merged commit f8e0b67 into PHPCSStandards:master Jan 5, 2024
38 checks passed
jrfnl pushed a commit that referenced this pull request Jan 5, 2024
* Generic/EmptyPHPStatement: add documentation

* Fix one of the invalid examples in EmptyPHPStatementStandard.xml
@rodrigoprimo rodrigoprimo deleted the empty-php-statement-documentation branch March 26, 2024 14:14
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