mildm8nnered
commented
3 weeks ago Should init and deinit be checked as well?
Closed Ueeek closed 3 days ago
mildm8nnered
commented
3 weeks ago Should init and deinit be checked as well?
SwiftLintBot
commented
1 week ago | 17 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 0.68s vs 0.7s on main (2% faster) |
| :book: | Linting Alamofire with this PR took 0.91s vs 0.92s on main (1% faster) |
| :book: | Linting Brave with this PR took 5.19s vs 5.12s on main (1% slower) |
| :book: | Linting DuckDuckGo with this PR took 2.91s vs 2.88s on main (1% slower) |
| :book: | Linting Firefox with this PR took 8.07s vs 8.09s on main (0% faster) |
| :book: | Linting Kickstarter with this PR took 6.8s vs 6.84s on main (0% faster) |
| :book: | Linting Moya with this PR took 0.42s vs 0.4s on main (4% slower) |
| :book: | Linting NetNewsWire with this PR took 1.81s vs 1.84s on main (1% faster) |
| :book: | Linting Nimble with this PR took 0.56s vs 0.57s on main (1% faster) |
| :book: | Linting PocketCasts with this PR took 6.09s vs 6.11s on main (0% faster) |
| :book: | Linting Quick with this PR took 0.34s vs 0.34s on main (0% slower) |
| :book: | Linting Realm with this PR took 3.42s vs 3.38s on main (1% slower) |
| :book: | Linting Sourcery with this PR took 1.72s vs 1.78s on main (3% faster) |
| :book: | Linting Swift with this PR took 3.13s vs 3.11s on main (0% slower) |
| :book: | Linting VLC with this PR took 0.91s vs 0.93s on main (2% faster) |
| :book: | Linting Wire with this PR took 12.11s vs 12.08s on main (0% slower) |
| :book: | Linting WordPress with this PR took 8.97s vs 8.88s on main (1% slower) |
Generated by :no_entry_sign: Danger
SimplyDanny
commented
1 week ago Such a check would actually also make sense for
deferblocks and emptystruct/class/... declarations. That can be up to an extension. However, to be prepared a name likeNoEmptyBlockRulewould leave room for these cases.
... and to catch blocks as well like requested in #5634.
Ueeek
commented
5 days ago @SimplyDanny Thank you for reviewing ~.
I fixed the review points.
Also, support catch and defer as well.
And change the rule name as suggested!
SimplyDanny
commented
4 days ago 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 private on the initializer is redundant.
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.
SimplyDanny
commented
4 days ago Any reason why
do {}
for _ in 1..<12 {}
while i < 12 {}should not trigger the rule?
Ueeek
commented
3 days ago @SimplyDanny Thank you for check again.
Instead of validating each syntax, validate CodeBlockSyntax so that we can support all type of CodeBlocks.(Accessor, DoCatch, Defer, Deinit, For, Function, if-else, init, repeat and while).
@mildm8nnered Thank you for mention about init.
As had discussion above, don't support accessibility modifiers on initializers for now.
Ueeek
commented
3 days ago @mildm8nnered @SimplyDanny Thank you for comments! Fixed all point!
SimplyDanny
commented
3 days ago Awesome first contribution! Thank you, @Ueeek.
marcelofabri
commented
3 days ago 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.
SimplyDanny
commented
3 days ago 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?
marcelofabri
commented
3 days ago 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.
SimplyDanny
commented
3 days ago 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 initializer_bodies (which includes deinitializers), function_bodies and statement_blocks (which includes if, for, defer, ...).
resolve #5615 Add
no_empty_functionrule