SwiftLint
SwiftLint copied to clipboard
realm
Add no blanket disables rule
Adds a new SwiftLint rule: no_blanket_disables
This warns (by default) when a SwiftLint rule is disabled, and then never later re-enabled. In many cases this is because the developer forgot to re-enable the rule, or somehow bodged it (e.g. // sswiftlint:enable some_rule), resulting in stretches of code going unintentionally un-linted.
Violations of this rule are surprisingly common. I found a bunch in the SwiftLint source code, which I've patched in various ways in this PR - some could simply be turned into // swiftlint:disable:next some_rule's and some I suppressed.
There are some rules that it doesn't make sense to re-enable, and that should apply to the whole file. An allowed_rules configuration parameter allows these to be specified. By default, file_length and single_test_class are allowed.
For these and possibly other rules, we may want to disable a particular rule always for the entire file, and to warn if it that rule is disabled with a modifier or re-enabled. The always_blanket_disable configuration parameter allows a set of rules to be specified that should always be warned on anything except a blanket disable. By default the set is empty.
This rule will also warn if you use swiftlint:disable to disable a rule that's already disabled, or swiftlint:enable to enable a rule that isn't disabled.
nonTriggeringExamples:
// swiftlint:disable unused_import
// swiftlint:enable unused_import
// swiftlint:disable unused_import unused_declaration
// swiftlint:enable unused_import
// swiftlint:enable unused_declaration
// swiftlint:disable:this unused_import")
// swiftlint:disable:next unused_import")
// swiftlint:disable:previous unused_import")
triggeringExamples:
// swiftlint:disable unused_import
// swiftlint:disable unused_import unused_declaration
// swiftlint:enable unused_import
// swiftlint:disable unused_import
// swiftlint:disable unused_import
// swiftlint:enable unused_import
// swiftlint:enable unused_import
See https://github.com/realm/SwiftLint/issues/4684
| 18 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 1.04s vs 1.04s on main (0% slower) |
| :book: | Linting Alamofire with this PR took 1.34s vs 1.34s on main (0% slower) |
| :book: | Linting Brave with this PR took 7.27s vs 7.25s on main (0% slower) |
| :book: | Linting DuckDuckGo with this PR took 3.11s vs 3.1s on main (0% slower) |
| :book: | Linting Firefox with this PR took 9.17s vs 9.12s on main (0% slower) |
| :book: | Linting Kickstarter with this PR took 10.07s vs 10.12s on main (0% faster) |
| :book: | Linting Moya with this PR took 0.52s vs 0.53s on main (1% faster) |
| :book: | Linting NetNewsWire with this PR took 3.02s vs 3.01s on main (0% slower) |
| :book: | Linting Nimble with this PR took 0.6s vs 0.58s on main (3% slower) |
| :book: | Linting PocketCasts with this PR took 7.07s vs 7.07s on main (0% slower) |
| :book: | Linting Quick with this PR took 0.23s vs 0.22s on main (4% slower) |
| :book: | Linting Realm with this PR took 11.32s vs 11.33s on main (0% faster) |
| :book: | Linting SourceKitten with this PR took 0.43s vs 0.43s on main (0% slower) |
| :book: | Linting Sourcery with this PR took 2.18s vs 2.18s on main (0% slower) |
| :book: | Linting Swift with this PR took 4.75s vs 4.75s on main (0% slower) |
| :book: | Linting VLC with this PR took 1.35s vs 1.33s on main (1% slower) |
| :book: | Linting Wire with this PR took 8.22s vs 8.19s on main (0% slower) |
| :book: | Linting WordPress with this PR took 10.98s vs 11.0s on main (0% faster) |
Generated by :no_entry_sign: Danger
In general, I like this rule a lot! The OSS findings and the findings in SwiftLint itself splendidly show how much SwiftLint misses it.
In general, I like this rule a lot! The OSS findings and the findings in SwiftLint itself splendidly show how much SwiftLint misses it.
Thanks! I just saw so many people just disabling stuff for the remainder of the file to fix one warning that it got annoying - we have a custom Danger script to do (some of) this, but much nicer to just have it baked in. And yes, it does seem to get a lot of hits :-)
Not entirely sure that no_blanket_disable is the best naming. Possibly blanket_disable_command, to echo superfluous_disable_command?
Indeed,
blanket_disable_commandsounds better to me. Also, the wording of the violation messages can be improved. I made a few suggestions. Please let me know what you think.
Cool - I went with blanket_disable_command and all your reason suggestions.
Just found some more points I'd like to discuss. Sorry! 😅
No worries :-) I'll try and get to these today ...
Just found some more points I'd like to discuss. Sorry! 😅
No worries :-) I'll try and get to these today ...
ok - so I think I've addressed everything from that last batch :-)