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/PHP/Syntax: add XML documentation #175

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Dec 19, 2023

Description

This PR adds documentation for the Generic.PHP.Syntax sniff.

I'm unsure about what to use in the title attribute. I followed the convention and used the sniff name, but just Syntax is not very clear. Happy to leave it as is if that is ok. An alternative that I considered using is PHP Syntax.

Suggested changelog entry

Add documentation for the Generic.PHP.Syntax 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.

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.

@rodrigoprimo Thanks for this PR. Looking good.

I'm unsure about what to use in the title attribute. I followed the convention and used the sniff name, but just Syntax is not very clear. Happy to leave it as is if that is ok. An alternative that I considered using is PHP Syntax.

I 100% agree with you. PHP Syntax would be better.

@jrfnl jrfnl added this to the 3.x Next milestone Dec 26, 2023
@rodrigoprimo rodrigoprimo force-pushed the php-syntax-documentation branch from 93e5c44 to 2245a6b Compare January 2, 2024 15:02
@rodrigoprimo
Copy link
Contributor Author

Thanks for the review, @jrfnl.

I updated the title to PHP Syntax and made the adjustments you suggested in the inline comments (#175 (comment)).

I amended the original commit instead of creating new ones per your comment in a previous PR: #163 (review). Please let me know if, in this case, you would have preferred new commits.

This PR is ready for another 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.

Thanks for making those changes @rodrigoprimo ! LGTM.

I amended the original commit instead of creating new ones per your comment in a previous PR: #163 (review). Please let me know if, in this case, you would have preferred new commits.

As a general rule of thumb:

  • For very small punctuation/doc fixes, amending is fine to keep the history clean.
  • For more significant/functional changes, a separate commit is preferred as that allows for a reviewer to see more easily what has been changed in the update.

When in doubt, use an extra commit (and I can then decide whether to squash the commits on merge or leave it as-is).

@jrfnl jrfnl merged commit fad8ffe into PHPCSStandards:master Jan 2, 2024
@rodrigoprimo rodrigoprimo deleted the php-syntax-documentation branch March 26, 2024 14:13
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