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

Jump to bottom

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

Merged
merged 3 commits into from
May 14, 2024
Merged

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

merged 3 commits into from
May 14, 2024

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented May 9, 2024 •
edited by jrfnl

Description

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

  • 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.
  • Avoid unnecessary extra calls to ArrayIndent::processMultiLineArray() when fixing an array closing brace that is not placed on its own line.
  • Remove T_COMMA from the list of tokens to ignore when looking for a token before the array opening brace. This was necessary in the past due to a bug File::findStartOfStatement() that has since been fixed.

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

  • 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
Copy link
Member

jrfnl commented May 13, 2024

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
Copy link
Contributor Author

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 jrfnl added this to the 3.10.0 milestone May 14, 2024
jrfnl
jrfnl approved these changes May 14, 2024
View reviewed changes
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.

LGTM! Thanks @rodrigoprimo !

@rodrigoprimo @jrfnl
Generic/ArrayIndent: remove T_COMMA from list of tokens to ignore
29b7411 
This commit removes T_COMMA from a list of tokens to ignore when
searching for the non-empty token before the start of the array. T_COMMA
was added to this list in 1697fac to fix a bug. See
squizlabs/PHP_CodeSniffer#3100 for more
details.

Back when this fix was introduced, it was necessary to ignore commas due
to a bug in findStartOfStatement() that would cause this method to
return the wrong token when passed the last token in a statement. This
bug has been fixed afterwards in ef80e53 and ignoring commas is not
necessary anymore. Now, the call to findStartOfStatement() in
https://github.com/squizlabs/PHP_CodeSniffer/blob/4cf6badaf0c177acaf295e2178f4383e2ea71b20/src/Standards/Generic/Sniffs/Arrays/ArrayIndentSniff.php#L67
correctly finds the start of the statement when passed the comma that
marks the end of the statement.

A test was added 1697fac. It would fail if running an older version of
PHPCS without ignoring commas, but now it passes:

https://github.com/squizlabs/PHP_CodeSniffer/pull/3101/files#diff-f786a9f6c41b82204dc89de2c02437c0e44401d580b02fcbde6278ea03f25693R83-R90
@rodrigoprimo @jrfnl
Generic/ArrayIndent: improve code coverage
d74b846
@rodrigoprimo @jrfnl
Generic/ArrayIndent: avoid extra calls to ArrayIndent::processMultiLi…
3a2230c 
…neArray()

This commit fixes a small inefficiency when fixing the
`CloseBraceNotNewLine` error. Previously, the sniff would add the newline
and the number of spaces used for an array element (which is more
than what should be used for the array closing brace). Then on a
subsequent call to processMultiLineArray(), it would detect the wrong
number of spaces for the closing brace and it would fix it. Now, the
sniff will use the correct number of spaces when adding the newline.
@jrfnl jrfnl force-pushed the test-coverage-array-indent branch from 906f7a2 to 3a2230c Compare May 14, 2024 17:36
@jrfnl
Copy link
Member

jrfnl commented May 14, 2024

Rebased without changes. Merging once the builds passes.

@jrfnl jrfnl merged commit a2c048b into PHPCSStandards:master May 14, 2024
40 checks passed
@rodrigoprimo rodrigoprimo deleted the test-coverage-array-indent branch May 14, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfnl jrfnl jrfnl approved these changes

Assignees
No one assigned
Labels
Standard: Generic Type: chores/QA
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rodrigoprimo @jrfnl