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

Correct pluralization of the word space/spaces depending on the spacing value #128

Conversation

DannyvdSluijs
Copy link
Contributor

@DannyvdSluijs DannyvdSluijs commented Dec 4, 2023

This PR fixes more occurences of the word space(s) to be either space or spaces depending on the spacing value.

Description

This PR is a port of squizlabs/PHP_CodeSniffer#3881 to this repo which was created after some brief discussion with @jrfnl in squizlabs/PHP_CodeSniffer#3647 and addressing the Sniff I've pointed out in squizlabs/PHP_CodeSniffer#3647 (review)

Suggested changelog entry

Correct pluralization of the word space/spaces depending on the spacing value

Related issues/external references

N/A

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.

@jrfnl jrfnl changed the title fix: Port https://github.com/squizlabs/PHP_CodeSniffer/pull/3881 to this repo Correct pluralization of the word space/spaces depending on the spacing value Dec 4, 2023
@jrfnl
Copy link
Member

jrfnl commented Dec 6, 2023

@DannyvdSluijs Could you please rebase your PR ? Status checks are required now and as some things have changed in the workflows we won't be able to get a passing build without a rebase.

@jrfnl jrfnl force-pushed the ImproveMessageReadabilityForMissingSpaces branch from 0b3c9af to 6e0457e Compare December 7, 2023 13:05
@jrfnl jrfnl added this to the 3.8.0 milestone Dec 7, 2023
DannyvdSluijs and others added 4 commits December 8, 2023 03:24
Correct pluralization of the word space/spaces depending on the spacing value.

For these sniffs, the changes are already covered by pre-existing tests.

Port squizlabs/PHP_CodeSniffer 3881 to this repo
Correct pluralization of the word space/spaces depending on the spacing value.

Includes adding tests for the `$indent` property being set to a 1 (open brace) and 0 (item indent, close brace).

Co-authored-by: Danny van der Sluijs <danny.van.der.sluijs@infi.nl>
Correct pluralization of the word space/spaces depending on the spacing value.

Includes adding tests for the `CloseBraceNotAligned` error code which would use the "1 space" phrasing.

Co-authored-by: Danny van der Sluijs <danny.van.der.sluijs@infi.nl>
Correct pluralization of the word space/spaces depending on the spacing value.

Includes adding tests for the `$requiredSpacesBeforeColon` property being set to a > 1 value.

Co-authored-by: Danny van der Sluijs <danny.van.der.sluijs@infi.nl>
@jrfnl jrfnl force-pushed the ImproveMessageReadabilityForMissingSpaces branch from 6e0457e to 5632363 Compare December 8, 2023 02:24
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 @DannyvdSluijs for porting this over. I review the changes again and will merge the PR now, including some additional changes.

Review notes:

  • Generic.Arrays.ArrayIndent
    • Has new tests covering 1/more than 1 spaces for KeyIncorrect and CloseBraceIncorrect error codes ✅
    • Missing test for OpenBraceIncorrect error code ❌
    • Missing tests with $indent set to 0 spaces. This is not necessarily a biggie for this PR, but something which should have been included in the tests for this sniff anyhow, so now is as good a time as any to fix this up ❌
    • Code review ✅
  • Generic.Formatting.SpaceAfterCast
    • Has (pre-existing) tests covering 0/1/more than 1 spaces for all affected error codes ✅
    • Code review ✅
  • Generic.Formatting.SpaceAfterNot
    • Has (pre-existing) tests covering 0/1/more than 1 spaces for all affected error codes ✅
    • Code review ✅
  • Squiz.Arrays.ArrayDeclaration
    • Has (partially new, partially pre-existing) tests 1/more than 1 spaces ✅
      No test with 0 spaces for most error codes, but that's okay as "spaces" is not configurable.
    • Code review ✅
  • Squiz.Commenting.DocCommentAlignment
    • Has (pre-existing) tests covering 1/more than 1 spaces ✅
      No test with 0 spaces, but that's okay as "spaces" is not configurable.
    • Code review ✅
  • Squiz.ControlStructures.ControlSignature
    • Has (pre-existing) tests covering the 0/1 spaces ✅
    • Missing test where (configurable) requiredSpacesBeforeColon property is set to > 1. ❌
    • Code review ✅

As I said I'd include this PR in the 3.8.0 release, I've taken the liberty to fix the missing tests up myself (which I normally wouldn't).

For future reference: I'd rather receive multiple PRs with each PR only addressing one sniff, as it makes reviewing easier and one change not being ready doesn't have to block other changes.

@jrfnl jrfnl merged commit 72ea7b3 into PHPCSStandards:master Dec 8, 2023
32 checks passed
jrfnl added a commit that referenced this pull request Dec 8, 2023
jrfnl added a commit that referenced this pull request Dec 8, 2023
@DannyvdSluijs DannyvdSluijs deleted the ImproveMessageReadabilityForMissingSpaces branch December 8, 2023 05:58
DannyvdSluijs pushed a commit to DannyvdSluijs/PHP_CodeSniffer_Continuation that referenced this pull request Apr 29, 2024
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.

None yet

2 participants