Include named closure parameters in closure end indentation rule #2132
Conversation
erichoracek
force-pushed
the
eh-close-end-indentation-params
branch
from
86b1b29
to
6450c92
Compare
Codecov Report
@@ Coverage Diff @@
## master #2132 +/- ##
==========================================
+ Coverage 91.86% 91.88% +0.02%
==========================================
Files 270 270
Lines 13220 13303 +83
==========================================
+ Hits 12144 12223 +79
- Misses 1076 1080 +4
Continue to review full report at Codecov.
|
|
IIRC this was a tradeoff that I made when writing the rule to avoid triggering too many warnings in cases like this where it's not really super clear what the "best" style is. Or even in cases like this where fixing the violation would be actually worse than the way it's right now. Maybe we should only make this check if the first parameter is in a new line? |
erichoracek
force-pushed
the
eh-close-end-indentation-params
branch
from
6450c92
to
8089815
Compare
|
@marcelofabri good call—I've updated this rule to only lint closure argument indentation if the first parameter is on a newline. PTAL! |
erichoracek
force-pushed
the
eh-close-end-indentation-params
branch
from
8089815
to
a5946ee
Compare
|
Looks great overall! I did find one false positive though: self.asdf(asdf: .asdf, asdfasdfasdf: {
asdfasdfasdfasdf })
{ result in
guard case .success = result else {
return
}
↓ self.asdfasdfasdf(asdfasdfasdfasdf: true) {
asdfasdfASDFasdfasdf
}
}code obviously obfuscated. |
CHANGELOG.md
Outdated
| @@ -19,6 +19,11 @@ | |||
| [Marcelo Fabri](https://github.com/marcelofabri) | |||
| [#2127](https://github.com/realm/SwiftLint/issues/2127) | |||
|
|
|||
| * Fixes a case where the `closure_end_indentation` rule wouldn't lint the end | |||
| indentation of non-trailing closure parameters. | |||
There was a problem hiding this comment.
two trailing spaces here please, as per https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes
erichoracek
force-pushed
the
eh-close-end-indentation-params
branch
from
a5946ee
to
26c36d4
Compare
|
@jpsim Thanks for the review! I tried pasting your false positive into the non-triggering examples section but I wasn't able to get it to trigger a test failure. I've pushed it up on a branch here—let me know if I'm missing something: Thanks in advance for taking a look! |
|
Sorry @erichoracek I made a mistake, there's no false positive. It was a false-false-positive 😅 I'm reviewing the rest now. |
Fixes #2121
Let me know if you think this belongs as a config parameter to this rule. Thanks in advance for taking a look!