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.

Sign up for GitHub

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 #3881

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf5cad1
fix: Correct pluralization of the word space/spaces depending on the …
DannyvdSluijs Aug 24, 2023
ed9d2b8
style: Correct code style violations
DannyvdSluijs Aug 25, 2023
2d51a63
Generic/ArrayIndent: add test covering the change
jrfnl Aug 27, 2023
a99c2d5
Squiz/ArrayDeclaration: test for CloseBraceNotAligned
jrfnl Aug 27, 2023
5e03f11
fix: Correct pluralization of the word space/spaces depending on the …
DannyvdSluijs Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/Standards/Generic/Sniffs/Arrays/ArrayIndentSniff.php
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sniff has three distinct error messages, which each use the "spaces" phrasing.
The last one (close brace) has now been adjusted, but the other two messages still have the same issue.

I also noted that there are no tests covering this change as indent 1 isn't used in the test file. While this wouldn't make a difference for the automated tests (as those don't test the actual error message), not having those tests there means that the change cannot easily be tested manually either and that code coverage goes down.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl thanks for the review and detailed messages. You mentioned the other two cases not being updated. This is a good spot and also shows that the words being used in the messages can be all over the place. Based on your other PR I searched the code base for spaces being mentioned in the error message. Searching for %s spaces in the codebase shows a total of 42 occurrences. This PR was drafted by searching for space(s) and fixing those.

What do you thing would be the desired approach here? Fix all thespaces occurrences using this same PR or target those for another PR (which I'm happy to create as well)

With regards to the coverage going done due the missing test that is actually somewhat of a valid point. I can look into adding these where possible. My only concern would be the amount of cases where I needed to tweak some of the config of class property values to get to the correct scenario to return the specific error message. Any tips you can share on how to approach that?

Also looking ahead at your other comment I guess answering this would help on that instance as well.
Happy to tweak the PR but some nudge into the correct direction would be helpful.

Copy link
Contributor

@jrfnl jrfnl Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think would be the desired approach here?

In my opinion, I'd keep this PR for the sniffs which are already being handled in it and would open separate PR(s) for changes to other sniffs. That way, this PR won't get blocked for merge by the new set of changes..

As a general rule of thumb, I normally try to keep PRs small and self-contained. That way, each PR can be reviewed and merged on their own merit. This prevents an incomplete or incorrect change to one file blocking the merge of the other changes, which may be perfectly ready to go.
It also makes it easier to review PRs, as a reviewer only needs to focus on one thing at a time and there is no risk of a PR getting blocked because it contains multiple different decision points (5 decisions: yes, 1: no, PR can't get merged).

Does that help ?

My only concern would be the amount of cases where I needed to tweak some of the config of class property values to get to the correct scenario to return the specific error message. Any tips you can share on how to approach that?

Well, to test the changes in the PR, I actually created some tests (locally) for the existing changes which weren't being hit by the tests. Would it help you if I would push those tests to this branch so you can see the tests I added (and how to change the sniff properties from within a test file) ?

Copy link
Author

@DannyvdSluijs DannyvdSluijs Aug 29, 2023 •
edited by jrfnl
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, valid points indeed. Most just wanted to make sure we are aligned here.

So I'll make sure that for any file already touched in this PR the issues with the words spaces is also addressed.

Would it help you if I would push those tests to this branch so you can see the tests I added (and how to change the sniff properties from within a test file) ?

Yes if you can please or if pushing is too much of a hassle feel free to drop some diff content here and I can take it from there as well.

Copy link
Contributor

@jrfnl jrfnl Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hassle, I check remote branches out locally anyway to test them. The tests should now show in your branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot! I just noticed the tests are failing now as I tested your changes in isolation (excluded the other error messages from the sniff). Let me fix that up. (I will force push the changes, so give me a moment before you pull the changes in)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've corrected all the plural cases on spaces in the files touched in this PR.
Looking at the tests and your additions I was mostly overwhelmed by the structure. How would you even check if specific cases are covered?

Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,15 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array
return;
}

$error = 'Array close brace not indented correctly; expected %s spaces but found %s';
$pluralizeSpace = 's';
if ($expectedIndent === 1) {
$pluralizeSpace = '';
}

$error = 'Array close brace not indented correctly; expected %s space%s but found %s';
$data = [
$expectedIndent,
$pluralizeSpace,
$foundIndent,
];
$fix = $phpcsFile->addFixableError($error, $arrayEnd, 'CloseBraceIncorrect', $data);
Expand Down
18 changes: 13 additions & 5 deletions src/Standards/Generic/Sniffs/Formatting/SpaceAfterCastSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,12 @@ public function register()
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$this->spacing = (int) $this->spacing;
$tokens = $phpcsFile->getTokens();
$this->spacing = (int) $this->spacing;
$pluralizeSpace = 's';
if ($this->spacing === 1) {
$pluralizeSpace = '';
}

if ($tokens[$stackPtr]['code'] === T_BINARY_CAST
&& $tokens[$stackPtr]['content'] === 'b'
Expand Down Expand Up @@ -83,8 +87,11 @@ public function process(File $phpcsFile, $stackPtr)

$nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
if ($nextNonEmpty !== $nextNonWhitespace) {
$error = 'Expected %s space(s) after cast statement; comment found';
$data = [$this->spacing];
$error = 'Expected %s space%s after cast statement; comment found';
$data = [
$this->spacing,
$pluralizeSpace,
];
$phpcsFile->addError($error, $stackPtr, 'CommentFound', $data);

if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
Expand All @@ -109,9 +116,10 @@ public function process(File $phpcsFile, $stackPtr)
return;
}

$error = 'Expected %s space(s) after cast statement; %s found';
$error = 'Expected %s space%s after cast statement; %s found';
$data = [
$this->spacing,
$pluralizeSpace,
$found,
];

Expand Down
18 changes: 13 additions & 5 deletions src/Standards/Generic/Sniffs/Formatting/SpaceAfterNotSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,12 @@ public function register()
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$this->spacing = (int) $this->spacing;
$tokens = $phpcsFile->getTokens();
$this->spacing = (int) $this->spacing;
$pluralizeSpace = 's';
if ($this->spacing === 1) {
$pluralizeSpace = '';
}

$nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($stackPtr + 1), null, true);
if ($nextNonEmpty === false) {
Expand All @@ -84,8 +88,11 @@ public function process(File $phpcsFile, $stackPtr)

$nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
if ($nextNonEmpty !== $nextNonWhitespace) {
$error = 'Expected %s space(s) after NOT operator; comment found';
$data = [$this->spacing];
$error = 'Expected %s space%s after NOT operator; comment found';
$data = [
$this->spacing,
$pluralizeSpace,
];
$phpcsFile->addError($error, $stackPtr, 'CommentFound', $data);
return;
}
Expand All @@ -101,9 +108,10 @@ public function process(File $phpcsFile, $stackPtr)
return;
}

$error = 'Expected %s space(s) after NOT operator; %s found';
$error = 'Expected %s space%s after NOT operator; %s found';
$data = [
$this->spacing,
$pluralizeSpace,
$found,
];

Expand Down
50 changes: 37 additions & 13 deletions src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php
View file
Open in desktop
Copy link
Contributor

@jrfnl jrfnl Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detailed notes:

  • The change for the CloseBraceNotAligned error code works, but has no tests covering the change (no test where the error message would result in "expected 1 space").
  • The other changes are all okay and covered by tests.
  • The KeyNotAligned error code has similar message phrasing, but the logic for building that error message has not been updated (yet). Note: there are tests already covering this, so it would only need the code change.

Original file line number Diff line number Diff line change
Expand Up @@ -330,11 +330,17 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array
}
} else if ($tokens[$arrayEnd]['column'] !== $keywordStart) {
// Check the closing bracket is lined up under the "a" in array.
$expected = ($keywordStart - 1);
$found = ($tokens[$arrayEnd]['column'] - 1);
$error = 'Closing parenthesis not aligned correctly; expected %s space(s) but found %s';
$data = [
$expected = ($keywordStart - 1);
$found = ($tokens[$arrayEnd]['column'] - 1);
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

$error = 'Closing parenthesis not aligned correctly; expected %s space%s but found %s';
$data = [
$expected,
$pluralizeSpace,
$found,
];

Expand Down Expand Up @@ -674,12 +680,18 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array
} else if ($previousIsWhitespace === true) {
$expected = $keywordStart;

$first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $valuePointer, true);
$found = ($tokens[$first]['column'] - 1);
$first = $phpcsFile->findFirstOnLine(T_WHITESPACE, $valuePointer, true);
$found = ($tokens[$first]['column'] - 1);
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

if ($found !== $expected) {
$error = 'Array value not aligned correctly; expected %s spaces but found %s';
$error = 'Array value not aligned correctly; expected %s space%s but found %s';
$data = [
$expected,
$pluralizeSpace,
$found,
];

Expand Down Expand Up @@ -783,11 +795,17 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array

$arrowStart = ($tokens[$indexPointer]['column'] + $maxLength + 1);
if ($tokens[$index['arrow']]['column'] !== $arrowStart) {
$expected = ($arrowStart - ($index['index_length'] + $tokens[$indexPointer]['column']));
$found = ($tokens[$index['arrow']]['column'] - ($index['index_length'] + $tokens[$indexPointer]['column']));
$error = 'Array double arrow not aligned correctly; expected %s space(s) but found %s';
$data = [
$expected = ($arrowStart - ($index['index_length'] + $tokens[$indexPointer]['column']));
$found = ($tokens[$index['arrow']]['column'] - ($index['index_length'] + $tokens[$indexPointer]['column']));
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

$error = 'Array double arrow not aligned correctly; expected %s space%s but found %s';
$data = [
$expected,
$pluralizeSpace,
$found,
];

Expand All @@ -801,7 +819,7 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array
}

continue;
}
}//end if

$valueStart = ($arrowStart + 3);
if ($tokens[$valuePointer]['column'] !== $valueStart) {
Expand All @@ -811,9 +829,15 @@ public function processMultiLineArray($phpcsFile, $stackPtr, $arrayStart, $array
$found = 'newline';
}

$error = 'Array value not aligned correctly; expected %s space(s) but found %s';
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

$error = 'Array value not aligned correctly; expected %s space%s but found %s';
$data = [
$expected,
$pluralizeSpace,
$found,
];

Expand Down
10 changes: 8 additions & 2 deletions src/Standards/Squiz/Sniffs/Commenting/DocCommentAlignmentSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,15 @@ public function process(File $phpcsFile, $stackPtr)
}

if ($tokens[$i]['column'] !== $requiredColumn) {
$error = 'Expected %s space(s) before asterisk; %s found';
$pluralizeSpace = 's';
if (($requiredColumn - 1) === 1) {
$pluralizeSpace = '';
}

$error = 'Expected %s space%s before asterisk; %s found';
$data = [
($requiredColumn - 1),
$pluralizeSpace,
($tokens[$i]['column'] - 1),
];
$fix = $phpcsFile->addFixableError($error, $i, 'SpaceBeforeStar', $data);
Expand All @@ -126,7 +132,7 @@ public function process(File $phpcsFile, $stackPtr)
$phpcsFile->fixer->replaceToken(($i - 1), $padding);
}
}
}
}//end if

if ($tokens[$i]['code'] !== T_DOC_COMMENT_STAR) {
continue;
Expand Down
18 changes: 15 additions & 3 deletions src/Standards/Squiz/Sniffs/ControlStructures/ControlSignatureSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,15 @@ public function process(File $phpcsFile, $stackPtr)
}

if ($found !== $expected) {
$error = 'Expected %s space(s) after %s keyword; %s found';
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

$error = 'Expected %s space%s after %s keyword; %s found';
$data = [
$expected,
$pluralizeSpace,
strtoupper($tokens[$stackPtr]['content']),
$found,
];
Expand All @@ -120,7 +126,7 @@ public function process(File $phpcsFile, $stackPtr)
$phpcsFile->fixer->replaceToken(($stackPtr + 1), str_repeat(' ', $expected));
}
}
}
}//end if

// Single space after closing parenthesis.
if (isset($tokens[$stackPtr]['parenthesis_closer']) === true
Expand All @@ -146,9 +152,15 @@ public function process(File $phpcsFile, $stackPtr)
}

if ($found !== $expected) {
$error = 'Expected %s space(s) after closing parenthesis; found %s';
$pluralizeSpace = 's';
if ($expected === 1) {
$pluralizeSpace = '';
}

$error = 'Expected %s space%s after closing parenthesis; found %s';
$data = [
$expected,
$pluralizeSpace,
$found,
];

Expand Down