Add new opt-in non_final_class rule
#6025
Conversation
JaviSoto
force-pushed
the
dev/javi/non-final-class
branch
from
8fb103d to
fc16f25
Compare
There was a problem hiding this comment.
Nice little rule that may have its use cases!
There are a few violations in SwiftLint's code base itself. They can likely be auto-fixed. However, if there is inheritance involved we shouldn't put an open but rather disable the rule locally or globally in the configuration (in case there are too many "false" positives).
Other than that, I've added a few remarks inline.
| struct NonFinalClassRule: Rule { | ||
| var configuration = SeverityConfiguration<Self>(.warning) | ||
|
|
||
| static let description = RuleDescription( |
There was a problem hiding this comment.
We now support adding rationals to rule descriptions. This feels like a very good candidate for some more explanation, as it seems highly opinionated and project-specific considering that open implies public which is often not what libraries want instead.
| } | ||
|
|
||
| private final class Visitor: ViolationsSyntaxVisitor<ConfigurationType> { | ||
| override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind { |
There was a problem hiding this comment.
Prefer visitPost instead.
| let modifiers = node.modifiers | ||
| if !modifiers.contains(keyword: .final), | ||
| !modifiers.contains(keyword: .open) { | ||
| let classToken = node.classKeyword |
There was a problem hiding this comment.
| let classToken = node.classKeyword | |
| let classTokenPosition = node.classKeyword.positionAfterSkippingLeadingTrivia |
| let classToken = node.classKeyword | ||
| violations.append(.init( | ||
| position: classToken.positionAfterSkippingLeadingTrivia, | ||
| reason: "Classes should be marked as `final` unless they are explicitly `open`", |
There was a problem hiding this comment.
Could be more succinct and targeted to the single class being checked:
| reason: "Classes should be marked as `final` unless they are explicitly `open`", | |
| reason: "Class should be marked as `final` or explicitly `open`", |
There was a problem hiding this comment.
This test isn't needed. All examples in the description are test cases by design.
| Example("public final class MyClass {}"), | ||
| ], | ||
| triggeringExamples: [ | ||
| Example("class MyClass {}"), |
There was a problem hiding this comment.
Please add violation markers ↓ to indicate the exact positions where the violations are expected to appear.
No description provided.