Fix false positives on closure rules on Swift 2.3 #1081
Conversation
Generated by 🚫 danger |
Current coverage is 83.23% (diff: 62.50%)@@ master #1081 diff @@
==========================================
Files 148 148
Lines 7215 7225 +10
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6010 6014 +4
- Misses 1205 1211 +6
Partials 0 0
|
There was a problem hiding this comment.
To test with a different toolchain that we're using to compile, we just need to set the $XCODE_DEFAULT_TOOLCHAIN_OVERRIDE environment variable when invoking the tests via xcodebuild or swift test, as documented in How is SourceKit Resolved?.
This could be added as a separate script in Travis.
But not all tests could be run in Swift 2.x mode, since some of them lint Swift 3 code (like the integration tests) so we'd also need to whitelist which tests to run.
Obviously that shouldn't be done in this PR, if at all.
| @@ -52,6 +52,11 @@ | |||
| [Marcelo Fabri](https://github.com/marcelofabri) | |||
| [#928](https://github.com/realm/SwiftLint/issues/928) | |||
|
|
|||
| * Fix false positives on `closure_parameter_position` and | |||
| `unused_closure_parameter` rules on Swift 2.3. | |||
There was a problem hiding this comment.
"with Swift 2.x" instead of "on Swift 2.3"
| @@ -27,7 +27,13 @@ extension Dictionary where Key: ExpressibleByStringLiteral { | |||
| } | |||
|
|
|||
| if SwiftDeclarationKind(rawValue: kindString) == .varParameter { | |||
| return [subDict] | |||
| let nestedParameters = subDict.enclosedVarParameters | |||
There was a problem hiding this comment.
Shouldn't this switch on the current Swift version instead?
| case three | ||
|
|
||
| static let current: SwiftVersion = { | ||
| let file = File(contents: "#sourceLocation()") |
There was a problem hiding this comment.
We should propose that a request type be added to SourceKit to get the current Swift version. But this is good for now.
| @@ -27,7 +27,13 @@ extension Dictionary where Key: ExpressibleByStringLiteral { | |||
| } | |||
|
|
|||
| if SwiftDeclarationKind(rawValue: kindString) == .varParameter { | |||
| return [subDict] | |||
| let nestedParameters = subDict.enclosedVarParameters | |||
| // on Swift 2.3, a closure parameter is inside another .varParameter and not inside an .argument | |||
There was a problem hiding this comment.
Generally "with Swift X.Y", not "on Swift X.Y".
|
@jpsim We can also create another scheme in Xcode changing the value of Besides the integration tests (that I've disabled), the only two currently failing tests are:
|
| @@ -23,4 +23,11 @@ enum SwiftVersion { | |||
|
|
|||
| fatalError("Unexpected Swift version") | |||
| }() | |||
|
|
|||
| var nameKey: String { | |||
There was a problem hiding this comment.
This is now used in just one place, keep it there? This shouldn't be a widely used thing.
There was a problem hiding this comment.
Actually this is being used on enclosedVarParameters as well 🤔
There was a problem hiding this comment.
But I could use "key.typename" directly there, as it's used inside the switch. What do you think would be better?
There was a problem hiding this comment.
I think nameKey isn't representative of the conditional value it returns. Copying the code inline avoids coming up with an accurate name for it. So either come up with a good name, or skirt around the issue :-)
There was a problem hiding this comment.
I've created a private nameKey(for version: SwiftVersion) function where it's really needed. I hope it's a good enough name as it's private 😅
This should fix #1019
Maybe we should have a way of running the tests on all Swift versions 🤔