PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
805 stars 47 forks source link

Generic/ArrayIndent: improve code coverage and implement a few small improvements to the sniff code #487

Closed rodrigoprimo closed 2 months ago

rodrigoprimo commented 2 months ago

Description

This PR improves the code coverage of the Generic.Arrays.ArrayIndent sniff and also implements the following minor improvements to the sniff code:

I believe T_DOUBLE_ARROW can be removed from the list as well. With T_COMMA, I could confirm that when this token was added to the ignore list, the provided test would fail without it. The test started passing when a bug in File::findStartOfStatement() was fixed.

I was not able to do the same with T_DOUBLE_ARROW. This token was added to the list in 0425342. All the tests pass if I run PHPCS in the 0425342 revision without the T_DOUBLE_ARROW token.

Related issues/external references

Part of #146

Types of changes

PR checklist

jrfnl commented 2 months ago

Use two different variables to hold the values of the expected indentation for array elements and array closing brace. Before, the same variable was being used, and its value modified, which can be confusing.

This change confuses me more. I mean, the $expectedIndent doesn't get changed in the loop, so the $expectedCloseIndent can just be set in the original location to $startIndent or better yet, doesn't even need a separate variable and can just use $startIndent. Why make things more complex than they need to be ?

rodrigoprimo commented 2 months ago

Thanks for checking this PR, @jrfnl.

This change confuses me more. I mean, the $expectedIndent doesn't get changed in the loop, so the $expectedCloseIndent can just be set in the original location to $startIndent or better yet, doesn't even need a separate variable and can just use $startIndent. Why make things more complex than they need to be ?

Good point about using $startIndent directly. I failed to consider this option. I dropped the commit that added the $expectedCloseBraceIndent variable and edited the commit that fixed the extra calls to use $startIndent.

jrfnl commented 2 months ago

Rebased without changes. Merging once the builds passes.