Squiz/SwitchDeclaration: bug fix - fixer conflict for single line code#1315
Merged
Squiz/SwitchDeclaration: bug fix - fixer conflict for single line code#1315
Conversation
While working on something else, I realized that the `Squiz.ControlStructures.SwitchDeclaration` sniff did not take into account that the "code under scan" could potentially be written to have various "parts" of a switch case/default structure on a single line.
This meant that, in that case, the sniff would have a fixer conflict with itself.
It would basically keep adding indentation whitespace for case/default/breaking statements not having the right indentation, but as it never added a new line _before_ the indentation, the indentation would still be incorrect in the next loop.
Fixed now by adding a number of extra checks to the sniff:
* `'ContentBefore' .$type` - which checks if there is anything but indentation whitespace, before a `case` or `default` keyword.
* `'ContentBeforeBreak'` - which checks if there is anything but indentation whitespace, before a `break/return` (etc) keyword.
* `'ContentAfter' .$type` - which checks if there is anything but a trailing comment, after a `case` or `default` statement.
The sniff also contains a couple of checks which verify the number of blank lines above/below certain statements. Those also didn't take into account that the count of the "lines between" may be zero, i.e. the code "after" being on the same line as the starting point for the check.
Those situations should now largely be fixed with the new lines being added via the `Content[Before|After]*` error codes.
For all other cases, the fixer code for the "number of blank lines" checks has been updated to handle trailing comments better, not leave trailing whitespace behind and not get into a fixer conflict with the new checks.
Notes:
* All fixers added/updated have been set up to prevent leaving trailing whitespace behind.
* The "fixed" code for one pre-existing test with a trailing comment has changed. In my opinion, the new "fixed" version is more correct, because:
1. This sniff is not about disallowing trailing comments, so this sniff should not act as if it has an opinion on that.
2. When moving (trailing) comments would be allowed, it could also result in tooling annotations being moved, which changes the meaning of the annotation in most cases.
* While working on this, I noticed various other fixes which are needed for this sniff. I've opened a ticket (1314) as a reminder to address these later.
While addressing those, the current changes may be made more efficient too.
Includes tests.
…is covered by tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
While working on something else, I realized that the
Squiz.ControlStructures.SwitchDeclarationsniff did not take into account that the "code under scan" could potentially be written to have various "parts" of a switch case/default structure on a single line.This meant that, in that case, the sniff would have a fixer conflict with itself.
It would basically keep adding indentation whitespace for case/default/breaking statements not having the right indentation, but as it never added a new line before the indentation, the indentation would still be incorrect in the next loop.
Fixed now by adding a number of extra checks to the sniff:
'ContentBefore' .$type- which checks if there is anything but indentation whitespace, before acaseordefaultkeyword.'ContentBeforeBreak'- which checks if there is anything but indentation whitespace, before abreak/return(etc) keyword.'ContentAfter' .$type- which checks if there is anything but a trailing comment, after acaseordefaultstatement.The sniff also contains a couple of checks which verify the number of blank lines above/below certain statements. Those also didn't take into account that the count of the "lines between" may be zero, i.e. the code "after" being on the same line as the starting point for the check.
Those situations should now largely be fixed with the new lines being added via the
Content[Before|After]*error codes.For all other cases, the fixer code for the "number of blank lines" checks has been updated to handle trailing comments better, not leave trailing whitespace behind and not get into a fixer conflict with the new checks.
Notes:
While addressing those, the current changes may be made more efficient too.
Includes tests.
Suggested changelog entry
Fixed: Squiz.ControlStructures.SwitchDeclaration: a number of the fixers would get into fixer conflicts with each other if the code under scan contained multiple statements on a line within a
switch.- To fix this, the sniff will now forbid - and auto-fix - multiple statements on one line for
case/defaultand "case breaking" statements.Types of changes