Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add First where rule #1012

Merged
merged 5 commits into from
Dec 22, 2016
Merged

Add First where rule #1012

merged 5 commits into from
Dec 22, 2016

Conversation

marcelofabri
Copy link
Collaborator

Fixes #1005.

I'm not sure if this should be a default rule, because it can trigger on types that have filter and first but not first(where:), for example Realm's Results.

@codecov-io
Copy link

codecov-io commented Dec 20, 2016

Current coverage is 82.72% (diff: 91.35%)

Merging #1012 into master will increase coverage by 0.04%

@@             master      #1012   diff @@
==========================================
  Files           140        141     +1   
  Lines          6832       6883    +51   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5649       5694    +45   
- Misses         1183       1189     +6   
  Partials          0          0          

Powered by Codecov. Last update ced652e...3d66012

@jpsim
Copy link
Collaborator

jpsim commented Dec 22, 2016

Yeah, I think there are other types either in Foundation or the Swift standard library that don't have first(where:) for which this rule would cause some false positives. I think the same reasons why empty_count is opt-in apply to this rule.

@jpsim
Copy link
Collaborator

jpsim commented Dec 22, 2016

Although if made opt-in, I'd definitely enable it in SwiftLint's own .swiftlint.yml.

@marcelofabri
Copy link
Collaborator Author

Although if made opt-in, I'd definitely enable it in SwiftLint's own .swiftlint.yml.

I thought I did that, sorry! Will update the PR

@jpsim
Copy link
Collaborator

jpsim commented Dec 22, 2016

This looks pretty good. What do you think of this?

diff --git a/Source/SwiftLintFramework/Rules/FirstWhereRule.swift b/Source/SwiftLintFramework/Rules/FirstWhereRule.swift
index 7d3041a..80ad7f5 100644
--- a/Source/SwiftLintFramework/Rules/FirstWhereRule.swift
+++ b/Source/SwiftLintFramework/Rules/FirstWhereRule.swift
@@ -47,19 +47,13 @@ public struct FirstWhereRule: OptInRule, ConfigurationProviderRule {
                 return nil
             }
 
-            let predicate = { (dictionary: [String: SourceKitRepresentable]) -> Bool in
+            return methodCallFor(bodyByteRange.location - 1, excludingOffset: firstByteRange.location,
+                                 dictionary: structure.dictionary, predicate: { dictionary in
                 guard let name = dictionary["key.name"] as? String else {
                     return false
                 }
-
                 return name.hasSuffix(".filter")
-            }
-
-            let callOffset = methodCallFor(bodyByteRange.location - 1,
-                                           excludingOffset: firstByteRange.location,
-                                           structure: structure,
-                                           predicate: predicate)
-            return callOffset
+            })
         }
 
         return violatingLocations.map {
@@ -71,36 +65,31 @@ public struct FirstWhereRule: OptInRule, ConfigurationProviderRule {
 
     private func methodCallFor(_ byteOffset: Int,
                                excludingOffset: Int,
-                               structure: Structure,
+                               dictionary: [String: SourceKitRepresentable],
                                predicate: ([String: SourceKitRepresentable]) -> Bool) -> Int? {
 
-        func parse(dictionary: [String: SourceKitRepresentable],
-                   predicate: ([String: SourceKitRepresentable]) -> Bool) -> Int? {
+        if let kindString = (dictionary["key.kind"] as? String),
+            SwiftExpressionKind(rawValue: kindString) == .call,
+            let bodyOffset = (dictionary["key.bodyoffset"] as? Int64).flatMap({ Int($0) }),
+            let bodyLength = (dictionary["key.bodylength"] as? Int64).flatMap({ Int($0) }),
+            let offset = (dictionary["key.offset"] as? Int64).flatMap({ Int($0) }) {
+            let byteRange = NSRange(location: bodyOffset, length: bodyLength)
 
-            if let kindString = (dictionary["key.kind"] as? String),
-                SwiftExpressionKind(rawValue: kindString) == .call,
-                let bodyOffset = (dictionary["key.bodyoffset"] as? Int64).flatMap({ Int($0) }),
-                let bodyLength = (dictionary["key.bodylength"] as? Int64).flatMap({ Int($0) }),
-                let offset = (dictionary["key.offset"] as? Int64).flatMap({ Int($0) }) {
-                let byteRange = NSRange(location: bodyOffset, length: bodyLength)
-
-                if NSLocationInRange(byteOffset, byteRange) &&
-                    !NSLocationInRange(excludingOffset, byteRange) && predicate(dictionary) {
-                    return offset
-                }
+            if NSLocationInRange(byteOffset, byteRange) &&
+                !NSLocationInRange(excludingOffset, byteRange) && predicate(dictionary) {
+                return offset
             }
+        }
 
-            if let subStructure = dictionary["key.substructure"] as? [SourceKitRepresentable] {
-                for case let dictionary as [String: SourceKitRepresentable] in subStructure {
-                    if let offset = parse(dictionary: dictionary, predicate: predicate) {
-                        return offset
-                    }
+        if let subStructure = dictionary["key.substructure"] as? [SourceKitRepresentable] {
+            for case let dictionary as [String: SourceKitRepresentable] in subStructure {
+                if let offset = methodCallFor(byteOffset, excludingOffset: excludingOffset,
+                                              dictionary: dictionary, predicate: predicate) {
+                    return offset
                 }
             }
-
-            return nil
         }
 
-        return parse(dictionary: structure.dictionary, predicate: predicate)
+        return nil
     }
 }

I'm asking because I'm not fully convinced myself, but there might be some ideas in there that apply.

@@ -35,7 +35,7 @@

* Add `first_where` opt-in rule that warns against using
`.filter { /* ... */ }.first` in collections, as
`.first(where: { /* ... */ })` is a more performant way.
`.first(where: { /* ... */ })` is often more efficient.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, bad rebase 😅

@jpsim
Copy link
Collaborator

jpsim commented Dec 22, 2016

🎉

@jpsim jpsim merged commit 590eb38 into realm:master Dec 22, 2016
@marcelofabri marcelofabri deleted the first_where branch December 22, 2016 17:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants