Vertical whitespace between cases #2292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2292 +/- ##
==========================================
+ Coverage 92.03% 92.07% +0.03%
==========================================
Files 302 303 +1
Lines 15180 15236 +56
==========================================
+ Hits 13971 14028 +57
+ Misses 1209 1208 -1
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
| @@ -15,6 +15,11 @@ | |||
| [Marcelo Fabri](https://github.com/marcelofabri) | |||
| [#2249](https://github.com/realm/SwiftLint/issues/2249) | |||
|
|
|||
| * Add new rule `vertical_whitespace_between_cases` to warn against crowded | |||
There was a problem hiding this comment.
I'd suggest to mention this is a opt-in rule.
|
|
||
| ```swift | ||
| switch x { | ||
| case 0..<5: |
There was a problem hiding this comment.
This triggering examples are not showing where the violation is triggered. Maybe this should be added, to match the other rules.
| print("x is invalid") | ||
| } | ||
| """: """ | ||
| switch x { |
There was a problem hiding this comment.
It's missing is the location where the violations are triggered. The examples should have "↓" to indicate where the violation is triggered. The Unit Tests will use the location of the arrow to make sure the violations are triggered on the correct place.
| return file.violatingRanges(for: pattern).map { | ||
| StyleViolation( | ||
| ruleDescription: type(of: self).description, | ||
| severity: ViolationSeverity.warning, |
There was a problem hiding this comment.
Most rules allow developers to set the severity of the violations triggered. One might want to set it to error instead or warning.
|
@ornithocoder Thank you for your feedback, I tackled all issues – please have another look! :) |
Jeehut
force-pushed
the
vertical-whitespace-between-cases
branch
3 times, most recently
from
420406a to
41debc9
Compare
|
I just rebased this to fix merge conflicts and updated to the new |
|
Is there anything else holding this off from being merged? |
Jeehut
force-pushed
the
vertical-whitespace-between-cases
branch
from
41debc9 to
b0704b1
Compare
|
Just rebased to solve merge conflicts. |
Jeehut
force-pushed
the
vertical-whitespace-between-cases
branch
from
b0704b1 to
39d74c6
Compare
Source/SwiftLintFramework/Rules/VerticalWhitespaceBetweenCasesRule.swift
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| extension VerticalWhitespaceBetweenCasesRule: OptInRule, AutomaticTestableRule { | ||
| public var configurationDescription: String { return "N/A" } |
There was a problem hiding this comment.
you shouldn't override this
There was a problem hiding this comment.
Overriding configurationDescription and init(configuration: Any) prevents the default violation severity configuration versions of these from working. I fixed this in this PR.
Source/SwiftLintFramework/Rules/VerticalWhitespaceBetweenCasesRule.swift
Outdated
Show resolved
Hide resolved
| public func validate(file: File) -> [StyleViolation] { | ||
| let patternRegex: NSRegularExpression = regex(pattern) | ||
|
|
||
| return file.violatingRanges(for: pattern).map { violationRange in |
There was a problem hiding this comment.
this should be done using the AST instead of a regex. You can check NoFallthroughOnlyRule which does something similar and the discussion on the PR: #2194
There was a problem hiding this comment.
Thanks for the pointer, looks like I'd need to basically rewrite the entire logic when switching to AstRule for which I don't have the time right now. I'll try to convert it when I have more time. Would you accept it as it is and we improve it later? I could leave a refactor comment ...
| kind: .style, | ||
| nonTriggeringExamples: Array(Set(violatingToValidExamples.values)) + nonTriggeringExamples, | ||
| triggeringExamples: Array(Set(violatingToValidExamples.keys)), | ||
| corrections: violatingToValidExamples.cleanedKeysDict() |
There was a problem hiding this comment.
you don't need to remove the markers from the keys here. It's actually better to leave them.
There was a problem hiding this comment.
When I let go off the cleanedKeysDict then 6 tests are failing. That's why I'm keeping it...
Jeehut
force-pushed
the
vertical-whitespace-between-cases
branch
from
39d74c6 to
935b4e2
Compare
|
Just rebased this branch onto current master. |
jpsim
force-pushed
the
vertical-whitespace-between-cases
branch
from
adde699 to
f6bbc91
Compare
|
Although I agree this would be better as an ASTRule, given that it's opt-in I see no issue with merging with the regex implementation. *However. That being said, SwiftLint spends most of its time executing regular expressions and we'll need to come up with a strategy for moving away from them if we want to improve SwiftLint's performance. |
Fixes #1517.
Please note that I made this rule opt-in for now, but I personally think this could also be turned on by default. When I ran the tests with this turned on by default (replacing
OptInRulewithRule) many files were updated (via autocorrect) and looked IMHO better afterwards.