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

Fix conflict within PSR12.ControlStructures.ControlStructureSpacing #581

Conversation

fredden
Copy link
Member

@fredden fredden commented Jul 29, 2024

Description

While looking into a fixer conflict for the PSR12 standard regarding the src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.1.inc file, it was noted that the fixer for PSR12.ControlStructures.ControlStructureSpacing was conflicting with itself.

For multi-line control structures, the first line of code must be on the next line after the control structure. This sniff correctly identified such cases. When the first line of code was on the same line as the control structure, the sniff correctly fixed this by adding a newline between these. However, when there were multiple blank lines between these, the fixer would continue adding new newlines.

This change fixes this bug by first removing all white-space before adding the one expected newline. Includes test.

Suggested changelog entry

PSR12.ControlStructures.ControlStructureSpacing: correctly handle multiple empty newlines between the start of a multi-line control structure and the next line of code.

Related issues/external references

#152

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.

@fredden fredden force-pushed the fixer-conflict/PSR12.ControlStructures.ControlStructureSpacing branch from 4baa479 to e67fe56 Compare July 29, 2024 21:43
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 working on this @fredden!

Looking good, I've just left two small comments inline for consideration.

For multi-line control structures, the first line of code must be on the next
line after the control structure. This sniff correctly identified such cases.
When the first line of code was on the same line as the control structure, the
sniff correctly fixed this by adding a newline between these. However, when
there were multiple blank lines between these, the fixer would continue adding
new newlines. This change fixes this bug by first removing all non-indentation
white-space before adding the one expected newline. Includes test.
@fredden fredden force-pushed the fixer-conflict/PSR12.ControlStructures.ControlStructureSpacing branch from d931828 to d9e164d Compare September 2, 2024 12:34
@fredden fredden requested a review from jrfnl September 2, 2024 12:37
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 @fredden for making those changes and adding the extra tests.

All good now. Merging once the build has passed.

@fredden fredden added this to the 3.10.x Next milestone Sep 2, 2024
@jrfnl jrfnl merged commit f905241 into PHPCSStandards:master Sep 2, 2024
40 checks passed
@fredden fredden deleted the fixer-conflict/PSR12.ControlStructures.ControlStructureSpacing branch September 2, 2024 13:22
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