-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Correct pluralization of the word space/spaces depending on the spacing value #128
Conversation
@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. |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
There was a problem hiding this 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
andCloseBraceIncorrect
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 ✅
- Has new tests covering 1/more than 1 spaces for
- 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 ✅
- Has (partially new, partially pre-existing) tests 1/more than 1 spaces ✅
- 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 ✅
- Has (pre-existing) tests covering 1/more than 1 spaces ✅
- 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.
This PR fixes more occurences of the word
space(s)
to be eitherspace
orspaces
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
PR checklist