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

Added PSR-2 documentation XML files #3832

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

blue32a
Copy link
Contributor

@blue32a blue32a commented May 20, 2023

Thank you for providing a great tool.

Description

I was looking into sniffs to create a coding standard for my project and it took me a long time to get to the generator.
I found some missing in the documentation and am submitting this PR.
Hope that helps.

Suggested changelog entry

  • Added missing PSR-2 documentation XML files.

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.
  • [Required for new files] I have added any new files to the package.xml file.

Copy link
Contributor

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

@blue32a Thank you for your willingness to contribute.

I have reviewed these XML docs, checked for common mistakes and tested them in CLI.

Findings:

  • PSR2.Files.ClosingTag: ✅
  • PSR2.Methods.FunctionClosingBrace: I'd recommend some minor tweaks to the code sample descriptions to make it more obvious what the sniff is looking for.
  • PSR2.Methods.FunctionCallSignature:
    • The code samples used aren't function calls, while the sniff is about function calls.
    • I'd recommend some tweaks to the code sample descriptions to make it more obvious what the sniff is looking for.
    • There are numerous additional checks the sniff does, but which are not mentioned in the docs.
      This is not a blocker, as those can be added at a later time, but it should be noted all the same.
      Extra checks currently not mentioned:
      • First argument in a multi-line function call must be on a new line.
      • Only one argument per line in a multi-line function call.
      • No blank lines in a multi-line function call.

Comment on lines 16 to 17
foo<em> </em>(<em> </em>$bar, $baz<em> </em>) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
foo<em> </em>(<em> </em>$bar, $baz<em> </em>) {
}
foo<em> </em>(<em> </em>$bar, $baz<em> </em>)<em> </em>;

@jrfnl
Copy link
Contributor

jrfnl commented Jun 8, 2023

P.S.: The new files should also be added to the package.xml file.

@blue32a
Copy link
Contributor Author

blue32a commented Jun 11, 2023

@jrfnl Thank you for your review.

I have updated the PR with your findings.

Copy link
Contributor

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

@blue32a Hiya, thanks for making those updates! The docs all look good to me now.

Only thing left to do is to update the package.xml file - as noted above, the three new files need to be added to the file list in the package.xml file.

@blue32a
Copy link
Contributor Author

blue32a commented Jun 12, 2023

I had missed it.
Three new documents have been added to package.xml.

Copy link
Contributor

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

I had missed it. Three new documents have been added to package.xml.

@blue32a Thank you! All looks good to me now ✅

@jrfnl jrfnl added this to the 3.8.0 milestone Jun 21, 2023
@jrfnl jrfnl merged commit 22edcb3 into squizlabs:master Jul 11, 2023
jrfnl added a commit that referenced this pull request Jul 11, 2023
jrfnl pushed a commit that referenced this pull request Jul 11, 2023
Documentation for the following PSR2 sniffs:
* `PSR2.Files.ClosingTag`
* `PSR2.Methods.FunctionCallSignature`
* `PSR2.Methods.FunctionClosingBrace`
@blue32a blue32a deleted the feature/add-psr2-docs branch July 11, 2023 08:56
@jrfnl
Copy link
Contributor

jrfnl commented Dec 8, 2023

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

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