Generic.WhiteSpaceDisallowSpaceIndent fixer bug when line only contains superfluous whitespace #1702
Conversation
…perfluous whitespace. When the `Generic.WhiteSpace.DisallowSpaceIndent` sniff encountered spaces on an otherwise empty line, the fixer would replace the spaces with tabs, remove the new line character and in effect prefix the whitespace to the next line. In other words, without the fix contains in this PR, this code snippet: ```php // The next line has four spaces. This is a specific test case. Do not remove the spaces. $bar = 2; ``` would be fixed as: ```php // The next line has four spaces. This is a specific test case. Do not remove the spaces. $bar = 2; ``` Includes unit test demonstrating the issue. The fix in this PR results in this sniff ignoring lines which only contain whitespace. Superfluous whitespace can be trimmed from lines using this `Squiz.WhiteSpace.SuperfluousWhitespace` sniff after all.
gsherwood
changed the title
…n line only contains superfluous whitespace
|
I had a go at fixing this another way. The DisallowTabIndent sniff replaces tabs with spaces in empty lines, so I thought it best that this sniff does the same. The superfluous whitespace can still be used to remove this whitespace, but it is no longer required. Let me know if it doesn't work for you though. |
Haven't tested yet, but looks ok to me. The one thing I'm wondering though: won't your fix pollute the metrics ? |
I wasn't too worried, but you made me worried. So I committed a change to stop the sniff recording empty lines. The tab and space indent sniffs were also reporting different metrics because they interpreted precision indentation different (space indenting really didn't care) so I change them to both report the proper metrics as well. |
I had previously done work on that as well - including an analysis of the differences in the metrics between the two sniffs - see open PRs #1599 and #1607, which also fix another issue with whitespace not being replaced in some cases. I'll rebase those two PRs and see what's left of the changes, shall I ? |
jrfnl
deleted the
feature/disallow-space-indent-properly-deal-with-blank-lines
branch
|
@gsherwood Ok, rebased both PR #1599 and #1607. Most of my metrics improvements in #1607 are still valid and will further improve the metrics output, though I have updated the PR to incorporate the improvements you already made. |
When the
Generic.WhiteSpace.DisallowSpaceIndentsniff encountered spaces on an otherwise empty line, the fixer would replace the spaces with tabs, remove the new line character and in effect prefix the whitespace to the next line.In other words, without the fix contains in this PR, this code snippet:
would be fixed as:
Includes unit test demonstrating the issue.
The fix in this PR results in this sniff ignoring lines which only contain whitespace.
Superfluous whitespace can be trimmed from lines using this
Squiz.WhiteSpace.SuperfluousWhitespacesniff after all.Originally reported in WordPress/WordPress-Coding-Standards#1192
N.B.: I am aware that this PR will cause a merge conflict in the test files with my other PR involving this sniff #1599. I will rebase whichever PR needs it once either one of them has been merged.
As the issues are unrelated, a separate PR to fix this issue seems warranted.