mildm8nnered
commented
7 months ago Obviously these lists can be quite subjective, but I think my personal breakdown would be something like this, without having looked at the actual violations (or even what some of the rules are). I don't have any attachment to particular rules, so if there are ones people disagree with ...
Things we probably do NOT want enabled:
| explicit_type_interface | yes | no | no | 3,878 | 0 | 3,878 | 541 |
| explicit_acl | yes | no | no | 3,065 | 0 | 3,065 | 535 |
| prefer_nimble | yes | no | no | 896 | 0 | 896 | 65 |
| explicit_top_level_acl | yes | no | no | 722 | 0 | 722 | 463 |
| required_deinit | yes | no | no | 625 | 0 | 625 | 318 |
| no_extension_access_modifier | yes | no | no | 0 | 550 | 550 | 274 |
| type_contents_order | yes | no | no | 452 | 0 | 452 | 282 |
| one_declaration_per_file | yes | no | no | 356 | 0 | 356 | 75 |
| no_grouping_extension | yes | no | no | 223 | 0 | 223 | 207 |
| sorted_enum_cases | yes | no | no | 132 | 0 | 132 | 29 |
| vertical_whitespace_between_cases | yes | yes | no | 109 | 0 | 109 | 45 |
| force_unwrapping | yes | no | no | 104 | 0 | 104 | 36 |
| switch_case_on_newline | yes | no | no | 97 | 0 | 97 | 18 |
| explicit_enum_raw_value | yes | no | no | 84 | 0 | 84 | 15 |
| multiline_function_chains | yes | no | no | 41 | 0 | 41 | 29 |Things we probably DO want enabled:
| implicit_return | yes | yes | no | 350 | 0 | 350 | 140 |
| prefer_self_in_static_references | yes | yes | no | 49 | 0 | 49 | 24 |
| multiline_parameters | yes | no | no | 39 | 0 | 39 | 26 |
| discouraged_optional_collection | yes | no | no | 33 | 0 | 33 | 20 |
| function_default_parameter_at_end | yes | no | no | 18 | 0 | 18 | 12 |
| closure_body_length | yes | no | no | 15 | 2 | 17 | 6 |
| todo | no | no | no | 11 | 0 | 11 | 8 |Things we MIGHT want enabled:
| indentation_width | yes | no | no | 1,833 | 0 | 1,833 | 333 |
| multiline_arguments_brackets | yes | no | no | 644 | 0 | 644 | 113 |
| no_magic_numbers | yes | no | no | 525 | 0 | 525 | 94 |
| multiline_arguments | yes | no | no | 213 | 0 | 213 | 54 |
| multiline_parameters_brackets | yes | no | no | 209 | 0 | 209 | 56 |
| file_types_order | yes | no | no | 190 | 0 | 190 | 73 |
| anonymous_argument_in_multiline_closure | yes | no | no | 185 | 0 | 185 | 64 |
| missing_docs | yes | no | no | 164 | 0 | 164 | 23 |
| multiline_literal_brackets | yes | no | no | 109 | 0 | 109 | 27 |
| conditional_returns_on_newline | yes | no | no | 76 | 0 | 76 | 50 |
| trailing_closure | yes | yes | no | 72 | 0 | 72 | 43 |
| convenience_type | yes | no | no | 59 | 0 | 59 | 54 |
| prefixed_toplevel_constant | yes | no | no | 53 | 0 | 53 | 33 |
| strict_fileprivate | yes | no | no | 51 | 0 | 51 | 21 |As @SimplyDanny pointed out in another context the other day, some violations can be reduced by configuration. For example, no_magic_numbers has an option to ignore violations in test cases, so configuring that would reduce it to about ~100 violations in non-test code.
SimplyDanny
% swiftlint --enable-all-rules --progress --reporter summaryon SwiftLint produces the following violations:Some of these rules we are unlikely to ever want on. For example,
explicit_type_interface(3,878 violations), but other, such asimplicit_return(350 violations) could actually be usefully on.For the latter cases, we could simply patch all of the existing violations and enable them, but we could also use a baseline to ignore (all of some of) the existing violations for now, but prevent any new violations of those rules being introduced.
Open questions would be:
1) Should we just patch all the violations of rules we want anyway.
In the case of some rules (e.g.
force_unwrapping) some case may be awkward to avoid in code.2) Assuming we don't just patch everything, which rules should we enable?
See my comment below for some suggestions on which rules we may want to enable.
3) How to run and configure SwiftLint?
SwiftLint is a bit weird, in that unlike most deployments of SwiftLint, there is no SwiftLint run during building - this can be a bit annoying, as you may not find out about violations until you run the integration tests, but is understandable - we would need a release build of SwiftLint to lint our local debug builds efficiently, and what version should that be - it can't be HEAD, because that's what we're building.
Instead, during Integration tests, SwiftLint lints itself (slowly) during the debug build. The proposal would be to use a baseline on this run.
As far as configuration goes, if we configure the baselines in
.swiftlint.yml(see #5552), then anyone runningswiftlintwill pick up the baseline configuration automatically. If we relied on command line configuration, then they would see all the violations that we are using the baseline to filter out.4) How to store the baseline
The baseline.json for the above violations is circa 7MB. If we just enable the rules suggested as DO or MIGHT enable, the baseline.json is about 2MB. Disabling the most frequent of those rules -
indentation_widthreduces the baseline size to about 1.3MBWe could store this uncompressed in the git tree, but over time, if we update it, it will increase the overall size of the repo history.
We could store it compressed, and then uncompress it during the Integration tests (and leave the uncompressed,
.gitignore'd version on disk), but then anyone just runningswiftlintwould get an error, as the uncompressed baseline would be missing.