Add new opt-in no_empty_block rule
#5617
Conversation
|
Should |
Ueeek
force-pushed
the
feature/no-empty-function-rule
branch
from
87b0f07
to
bb520cd
Compare
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyFunctionRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Style/VerticalWhitespaceOpeningBracesRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyFunctionRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyFunctionRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyFunctionRule.swift
Outdated
Show resolved
Hide resolved
... and to |
|
@SimplyDanny Thank you for reviewing ~. |
CHANGELOG.md
Outdated
| @@ -15,6 +15,9 @@ | |||
| * Linting got around 20% faster due to the praisworthy performance | |||
| improvements done in the [SwiftSyntax](https://github.com/apple/swift-syntax) | |||
| library. | |||
| * Add `no_empty_block` default rule to validate that `function`, `init`, `deinit`, `catch` and `defer` should not have empty code block. They should at least contain a comment. | |||
There was a problem hiding this comment.
Probably want to wrap this text the same as the rest of the file
| @@ -3,7 +3,7 @@ public struct Stack<Element> { | |||
| private var elements = [Element]() | |||
|
|
|||
| /// Creates an empty `Stack`. | |||
| public init() {} | |||
| public init() { /* Publish no-op initializer */ } | |||
There was a problem hiding this comment.
If this is a legitimate exception, because it's making the constructor public, could we detect and ignore it. I think this would only apply to init as far as I can think of.
| @@ -12,7 +12,7 @@ public final class RuleRegistry: @unchecked Sendable { | |||
| /// accessed will not work. | |||
| public private(set) lazy var list = RuleList(rules: registeredRules) | |||
|
|
|||
| private init() {} | |||
| private init() { /* To guarantee that this is singleton. */ } | |||
There was a problem hiding this comment.
Also maybe this could be an exception too
| @@ -1,7 +1,7 @@ | |||
| package struct SuperfluousDisableCommandRule: SourceKitFreeRule { | |||
| package var configuration = SeverityConfiguration<Self>(.warning) | |||
|
|
|||
| package init() {} | |||
| package init() { /* Make initializer as accessible as its type. */ } | |||
There was a problem hiding this comment.
Maybe here too - if an init has any accessibility modifier apart from internal, then there is a good reason it might be empty.
|
Regarding accessibility modifiers on initializers: While it might be true that explicit modifiers have a potential meaning and are there for a good reason, it's not too easy to find out if the modifier is really required. See how I struggle in #5048 for example and consider private struct S {
private init() {}
}where the So in the first shot of this rule, I'd require the comment in every empty block disregarding access control modifiers. It also doesn't hurt to enforce an explaining comment for these cases. Later we can add this specialization optionally. |
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyBlockRule.swift
Outdated
Show resolved
Hide resolved
|
Any reason why do {}
for _ in 1..<12 {}
while i < 12 {}should not trigger the rule? |
|
@SimplyDanny Thank you for check again. @mildm8nnered Thank you for mention about |
Source/SwiftLintBuiltInRules/Rules/Idiomatic/NoEmptyBlockRule.swift
Outdated
Show resolved
Hide resolved
Ueeek
force-pushed
the
feature/no-empty-function-rule
branch
from
40c3b86
to
73e3d95
Compare
|
@mildm8nnered @SimplyDanny Thank you for comments! Fixed all point! |
|
Awesome first contribution! Thank you, @Ueeek. |
SimplyDanny
changed the title
no_empty_block rule
|
I think it’s a bad idea to enable this rule by default. Besides the initializer cases that were caught here, a common case would be creating mock types that need to conform to a protocol, but don’t care about a specific method. |
I have no strong opinion on that. In my world, all rules could be optional and I enable them specifically. However, it seems like people are often not aware of new optional rules. That's unfortunate. So having some default rules (with a low false positive rate) is actually a nice way to avoid helpful and likeable rules being unused. Mocks usually appear in test code. Isn't it fine then to disable the rule in these files? |
It’s a big ask to disable the rule in some files in a large enough codebase. The number of violations in OSS check tells me this should be opt-in. |
What about adding a configuration that allows to disable the rule for certain constructs? Options could be |
resolve #5615
Add
no_empty_functionrule