Generic/OpeningFunctionBraceKernighanRichie: improve test coverage #707

rodrigoprimo · 2024-11-21T20:15:12Z

Description

This PR improves code coverage for the Generic.Functions.OpeningFunctionBraceKernighanRichie sniff and makes some tiny improvements to the sniff code. No user-facing changes were made.

Related issues/external references

Part of #146

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

@rodrigoprimo Thanks for this PR. I think it's a great start, but can be improved further.

Looking at the tests, I see the following:

Missing:

Test with an abstract method (no scope opener).
Live coding test missing the opening curly.
Live coding test with an open curly, but missing the close curly x 3 - one with correct spacing, one with too much space before the curly, one with no space before the curly. Both these last two should be fixable (and fixed).

Superfluous:

Live coding test missing open parentheses. The sniff is not looking at that.

Looking at the handling of tabs... I don't think the multi-tab situation is handled correctly.

If you run the sniff against test case file 2, you will get the following error for the multi-tab line:

 10 | ERROR | [x] Expected 1 space before opening brace; found 3 (Generic.Functions.OpeningFunctionBraceKernighanRitchie.SpaceBeforeBrace)

... which has no indication that this is 3 tabs, not 3 spaces.

If tabs are called out separately anyway (as is done in this sniff), let's do it correctly and consistently.

And if we're talking redundant code, I think the whole code block from line 75 to line 82 can be removed as long as the $closeBracket reference on line 85 is replaced with null.

What do you think ?

src/Standards/Generic/Tests/Functions/OpeningFunctionBraceKernighanRitchieUnitTest.1.inc

src/Standards/Generic/Tests/Functions/OpeningFunctionBraceKernighanRitchieUnitTest.3.inc

rodrigoprimo · 2024-11-26T20:00:06Z

Thanks for your detailed review, @jrfnl!

Test with an abstract method (no scope opener).

There is an abstract method declared in an interface, and I assumed it was enough as the sniff doesn't really check where the method is defined. That being said, having an abstract method declared in a class is not a problem, so I went ahead and added one.

Live coding test missing the opening curly.

👍

Live coding test with an open curly, but missing the close curly x 3 - one with correct spacing, one with too much space before the curly, one with no space before the curly. Both these last two should be fixable (and fixed).

I might be missing something, but it seems to me the sniff will bail early in those three cases, as when there is no closing curly brace, scope_opener is not set.

This is one of the code examples that I have in mind reading your comment:

function missingClosingCurlyBrace()     {

I'm assuming you are not suggesting that I change the behavior of the sniff to handle those cases.

Live coding test missing open parentheses. The sniff is not looking at that.

I replaced this test with a test for the missing opening curly brace.

Looking at the handling of tabs... I don't think the multi-tab situation is handled correctly.
If you run the sniff against test case file 2, you will get the following error for the multi-tab line:
10 | ERROR | [x] Expected 1 space before opening brace; found 3 (Generic.Functions.OpeningFunctionBraceKernighanRitchie.SpaceBeforeBrace)
... which has no indication that this is 3 tabs, not 3 spaces.
If tabs are called out separately anyway (as is done in this sniff), let's do it correctly and consistently.

I agree, but I don't see an easy and straightforward way to fix it. Especially as there could be a mix of spaces and tabs used. Would it be ok to open an issue for this instead of fixing it in this PR?

And if we're talking redundant code, I think the whole code block from line 75 to line 82 can be removed as long as the $closeBracket reference on line 85 is replaced with null.
What do you think ?

Good catch! I removed it in a new commit.

jrfnl · 2024-11-27T09:42:07Z

Live coding test with an open curly, but missing the close curly x 3 - one with correct spacing, one with too much space before the curly, one with no space before the curly. Both these last two should be fixable (and fixed).

I might be missing something, but it seems to me the sniff will bail early in those three cases, as when there is no closing curly brace, scope_opener is not set.

This is one of the code examples that I have in mind reading your comment:
function missingClosingCurlyBrace()     {
I'm assuming you are not suggesting that I change the behavior of the sniff to handle those cases.

Well, sort of. I mean, this sniff is limiting its own ability by checking for the scope_opener instead of checking for a T_OPEN_CURLY_BRACKET after the function signature.

Having said that, I agree that should probably be left as "future scope" (and low priority).

Looking at the handling of tabs... I don't think the multi-tab situation is handled correctly.
If you run the sniff against test case file 2, you will get the following error for the multi-tab line:
10 | ERROR | [x] Expected 1 space before opening brace; found 3 (Generic.Functions.OpeningFunctionBraceKernighanRitchie.SpaceBeforeBrace)
... which has no indication that this is 3 tabs, not 3 spaces.
If tabs are called out separately anyway (as is done in this sniff), let's do it correctly and consistently.

I agree, but I don't see an easy and straightforward way to fix it. Especially as there could be a mix of spaces and tabs used. Would it be ok to open an issue for this instead of fixing it in this PR?

I do see a straight-forward way to fix this, so I will submit a follow-up PR after merging this one. No need for the issue.

…findPrevious() This commit removes a repeated call to findPrevious() to get the pointer to the end of the function declaration. The same `$prev` variable that was defined a few lines above can be reused and there is no need to define it again calling `findPrevious()` with exactly the same parameters.

… in a fixer This commit makes a tiny performance improvement to the sniff when handling fixing code with tabs. Before this change, the sniff would need two passes of the fixer for code with one tab before the opening brace. First, it would add a space between the tab and the opening brace. Then, on a second pass, it would replace the tab and the space with just one space. Now, it replaces the tab with the space directly without the need for a second pass.

Doing this to be able to create tests with syntax errors on separate files.

Includes removing unnecessary trailing spaces from empty lines in the test file.

…end of the function signature When finding the end of the function signature, the sniff had an unnecessary block of code to change the `$end` parameter passed to `findPrevious()` when handling a closure with a `use` statement. Since we are looking for the first non-empty token before the opening curly brace, it is not necessary to change the value of the `$end` parameter. Actually, we don't even need to limit the search and we can pass `null` (the default value). The returned first non-empty token will be the same anyway regardless if `$end` is the closing parenthesis before `use`, after `use` or `null`.

jrfnl

Thanks for those updates @rodrigoprimo.

I'm going to rebase the PR and squash a few commits together. Merging once the build has passed.

github-actions bot added Standard: Generic Status: triage labels Nov 21, 2024

rodrigoprimo force-pushed the test-converage-opening-function-brace-kernighan-ritchie branch from ea6c6ec to f8639a2 Compare November 21, 2024 20:17

rodrigoprimo added Type: chores/QA and removed Status: triage labels Nov 22, 2024

jrfnl requested changes Nov 25, 2024

View reviewed changes

src/Standards/Generic/Tests/Functions/OpeningFunctionBraceKernighanRitchieUnitTest.1.inc Outdated Show resolved Hide resolved

src/Standards/Generic/Tests/Functions/OpeningFunctionBraceKernighanRitchieUnitTest.3.inc Outdated Show resolved Hide resolved

jrfnl added the Status: awaiting feedback label Nov 25, 2024

rodrigoprimo force-pushed the test-converage-opening-function-brace-kernighan-ritchie branch from e4f38f1 to 0111d93 Compare November 26, 2024 19:59

rodrigoprimo removed the Status: awaiting feedback label Nov 26, 2024

rodrigoprimo added 5 commits November 27, 2024 10:46

Generic/OpeningFunctionBraceKernighanRichie: rename test case file

111d851

Doing this to be able to create tests with syntax errors on separate files.

Generic/OpeningFunctionBraceKernighanRichie: improve test coverage

195346f

Includes removing unnecessary trailing spaces from empty lines in the test file.

jrfnl approved these changes Nov 27, 2024

View reviewed changes

jrfnl force-pushed the test-converage-opening-function-brace-kernighan-ritchie branch from 0111d93 to 286cd81 Compare November 27, 2024 09:49

jrfnl added this to the 3.11.2 milestone Nov 27, 2024

jrfnl merged commit 941a00e into PHPCSStandards:master Nov 27, 2024
45 checks passed

jrfnl deleted the test-converage-opening-function-brace-kernighan-ritchie branch November 27, 2024 10:21

jrfnl mentioned this pull request Nov 27, 2024

Generic/OpeningFunctionBraceKernighanRitchie: improve error message #736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic/OpeningFunctionBraceKernighanRichie: improve test coverage #707

Generic/OpeningFunctionBraceKernighanRichie: improve test coverage #707

rodrigoprimo commented Nov 21, 2024

jrfnl left a comment

rodrigoprimo commented Nov 26, 2024

jrfnl commented Nov 27, 2024

jrfnl left a comment •

edited

Loading

Generic/OpeningFunctionBraceKernighanRichie: improve test coverage #707

Generic/OpeningFunctionBraceKernighanRichie: improve test coverage #707

Conversation

rodrigoprimo commented Nov 21, 2024

Description

Related issues/external references

Types of changes

PR checklist

jrfnl left a comment

Choose a reason for hiding this comment

rodrigoprimo commented Nov 26, 2024

jrfnl commented Nov 27, 2024

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

jrfnl left a comment •

edited

Loading