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/ArrayDeclaration: fixes false positive when handling short lists inside foreach #748

Merged

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Nov 29, 2024

Description

This PR fixes a false positive when handling a short list inside a foreach as reported in #527. I implemented the solution that @jrfnl described in this comment: #527 (comment).

I included a separate commit with some tests that I wrote to improve test coverage for this sniff while I was familiarizing myself with its code. This is not a full code coverage commit. There are still some uncovered lines in this sniff, and those lines might be unreachable, but I have not investigated this at this time. There are also potentially some more relevant tests that could be added for lines that are already covered. This is also not addressed in this commit.

cc @VladaHejda in case you are available to test this PR and confirm if it fixes the problem that you reported. Thanks.

Suggested changelog entry

Squiz.Arrays.ArrayDeclaration: fix false positive when handling short lists inside foreach

Related issues/external references

Fixes #527

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.

Thanks for this PR @rodrigoprimo!

Looking good. I have only left a few small remarks inline.

I included a separate commit with some tests that I wrote to improve test coverage for this sniff while I was familiarizing myself with its code.

I appreciate the additional tests, but for the record: these should have been pulled in a separate PR as they have nothing to do with the issue you are addressing in this PR.

@rodrigoprimo
Copy link
Contributor Author

Thanks for your review, @jrfnl. I believe I replied to all your comments, and this PR is ready for another check.

I appreciate the additional tests, but for the record: these should have been pulled in a separate PR as they have nothing to do with the issue you are addressing in this PR.

Noted. Please let me know if you prefer that I move the commit with the additional tests to a separate PR in this case.

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 finishing this off @rodrigoprimo, all good now.

@jrfnl jrfnl force-pushed the fix-array-declaration-short-list-bug branch from d8bc393 to ad4b14a Compare December 7, 2024 10:33
@jrfnl jrfnl force-pushed the fix-array-declaration-short-list-bug branch from ad4b14a to 48f1737 Compare December 7, 2024 10:45
This commit adds tests that were previously missing. There are still
some uncovered lines in this sniff, those lines might be unreachable,
but I did not investigate this at this time. There is also potentially
some more relevant tests that could be added for lines that are already
covered. This is also not addressed in this commit.
This commit improves how the `Squiz.Arrays.ArrayDeclaration` sniff
handles short lists inside a foreach and fixes a false positive.

See PHPCSStandards#527
@jrfnl jrfnl force-pushed the fix-array-declaration-short-list-bug branch from 48f1737 to 8e0074a Compare December 7, 2024 10:59
@jrfnl jrfnl merged commit 0d969c9 into PHPCSStandards:master Dec 7, 2024
45 checks passed
@jrfnl jrfnl deleted the fix-array-declaration-short-list-bug branch December 7, 2024 11:28
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.

Array destructuring in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma
2 participants