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

Squiz.Commenting.ClosingDeclarationComment - add missing .fixed file for test-suite, and fix a bug found during #340

Conversation

fredden
Copy link
Member

@fredden fredden commented Feb 15, 2024

Description

This pull request adds a missing .fixed file for an existing test for Squiz.Commenting.ClosingDeclarationComment.

While I was reviewing the automatically-created ".fixed" file (from phpcbf), I noticed a bug. I'm fixing that bug in this same pull request, but am happy to move that elsewhere if desired.

Suggested changelog entry

  • Squiz.Commenting.ClosingDeclarationComment - no longer add extra newline when adding a missing comment

Related issues/external references

Related to #299

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

@fredden
Copy link
Member Author

fredden commented Feb 15, 2024

@jrfnl is it too late to include this in the upcoming 3.9.0 release? Ref #341.

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

@jrfnl jrfnl added this to the 3.9.0 milestone Feb 15, 2024
@jrfnl
Copy link
Member

jrfnl commented Feb 15, 2024

@jrfnl is it too late to include this in the upcoming 3.9.0 release? Ref #341.

Nah, I'll include it 😉

While reviewing this change I did notice a number of other things, but those don't need to be addressed in this PR.

  • No tests for a Missing function closing comment.
  • No tests with a misplaced comment with indentation.
  • No tests with a misplaced comment with multiple whitespace tokens between the close brace and the comment. (multiple new lines)
  • No tests for close brace at the very end of the file (without a new line) - this would need one or more extra test case files to be tested properly.
    This would also expose a (non-problematic) bug on line 96, the if (rtrim($tokens[$next]['content']) === $comment) condition is buggy as $next can be false and that's not correctly taken into account.
    This doesn't lead to problems as the contents of token 0 will be compared to the expected comment, which can only be the PHP open tag or inline HTML and only if //end ... would be found in inline HTML could this lead to false positives, though that would make for invalid HTML anyway.
  • There is some dead code - the if ($closingBracket === null) code block can be removed as the $closingBracket can never be null as the sniff already bows out before setting the variable on a isset($tokens[$stackPtr]['scope_closer']) === false condition.
  • I wonder if the sniff should also handle traits ? I.e. require a comment on the trait closer. Would make sense for consistency.
  • Might be good to document (via a test) that anonymous classes and arrow functions do not require a close comment (same as closures, which is already documented via a test).

We should either open an issue about these or if you're up for it, open a follow-up PR to address these issues.

@jrfnl jrfnl merged commit 4bbc820 into PHPCSStandards:master Feb 15, 2024
40 checks passed
@fredden fredden deleted the missing-fixed-file/Squiz.Commenting.ClosingDeclarationComment branch February 16, 2024 11:03
@fredden
Copy link
Member Author

fredden commented Feb 16, 2024

Yes, I noticed a lot of untested cases while looking for the bug that I fixed here. I decided not to fix all of issues that I noticed (which you've now documented above - thanks) in this pull request, to keep the scope small. I thought that we can re-assess this sniff in scope of #146 when we get time for that.

@jrfnl
Copy link
Member

jrfnl commented Feb 16, 2024

@fredden I agree, those other things can be addressed in a separate PR(s), but I do think they need to be addressed.

Bullet 1, 2, 3 + the last one are related to #146. The other bullets are functional concerns/changes which are needed though.

I'm going to link my comment in #146 for visibility.

@rodrigoprimo
Copy link
Contributor

rodrigoprimo commented Feb 22, 2024

Hi, @fredden! I was talking with @jrfnl about things that I could do to help the PHPCS project, and she mentioned that I could work on the items from the list above. But first, she suggested I check with you to see if you are working or planning to work on those items? Please let me know if you prefer to work on those or if you would like help. Thanks.

@fredden
Copy link
Member Author

fredden commented Feb 22, 2024

@rodrigoprimo thanks for checking in. You're welcome to work on those; they're not currently on my list.

@rodrigoprimo
Copy link
Contributor

Thanks, @fredden. I will work on it next week.

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.

3 participants