Trailing closure rule #1193
Trailing closure rule #1193
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1193 +/- ##
==========================================
+ Coverage 87.35% 87.41% +0.06%
==========================================
Files 207 208 +1
Lines 10206 10278 +72
==========================================
+ Hits 8915 8985 +70
- Misses 1291 1293 +2
Continue to review full report at Codecov.
|
|
I'm not sure about this one. |
About this being a rule at all or being a default rule? I feel like it should be opt-in since it should trigger lots of warnings on projects and there's no definitive decision from the community. |
|
Even as opt-in I think it needs work. For example, I wouldn't consider accessing the property after a call to a closure to require it to use trailing closure syntax, such as the first OSSCheck violation reported here: XCTAssertEqual(1, schema.objectSchema.filter({ $0.className == "SwiftStringObject" }).count) |
|
I think that could be a configuration, since I can see teams preferring one style over another. |
😳 . Should definitely be opt-in. |
|
does it support the case where we would have an ambiguity between two func? |
|
@Coeur not really, and I'm not sure we'd able to address that. Another reason to make this opt-in. |
|
@marcelofabri what would you like to do with this? |
|
I can revive this, just want to be sure what the behavior should be regarding #1193 (comment) |
|
Not sure if you guys already considered this, but just putting it out there. Trailing closure syntax doesn't work within a conditional statement: 4> if a.filter { $0 == 1 }.isEmpty {
5. print("no ones!")
6. }
error: repl.swift:4:15: error: anonymous closure argument not contained in a closure
if a.filter { $0 == 1 }.isEmpty {
^
error: repl.swift:4:24: error: consecutive statements on a line must be separated by ';'
if a.filter { $0 == 1 }.isEmpty {
^
;
4> if a.filter({ $0 == 1 }).isEmpty {
5. print("no ones!")
6. } |
|
@marcelofabri if the question is:
Then second syntax is fine. But again, I would personally not opt-in to this rule, because of the false positive this rule would cause like in the example I gave two months ago, so my opinion is biased. |
|
@Mazyod the rule already deals with it |
|
@jpsim I'm thinking about merging this one as it is. Later we can add a configuration to deal with #1193 (comment). |
|
Sounds good @marcelofabri! |
|
Should we file an issue to allow parameterizing the rule to allow/disallow #1193 (comment)? |
|
Filled #1718. |
Fixes #54
I'm thinking maybe this should be opt-in because it's not clear what style the community prefers.
I'm curious to see the OSS check on this rule.
One improvement that could be made is ignore a violation if there're other closure parameters in the call.