Add rule requiring imports be sorted #991

sberrevoets · 2016-12-15T17:42:59Z

Adds an opt-in rule to requiring imports to be sorted, as requested in #900.

codecov-io · 2016-12-15T18:32:08Z

Current coverage is 82.53% (diff: 92.85%)

Merging #991 into master will increase coverage by 0.45%

@@             master       #991   diff @@
==========================================
  Files           136        139     +3   
  Lines          6413       6694   +281   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5264       5525   +261   
- Misses         1149       1169    +20   
  Partials          0          0

Powered by Codecov. Last update a056b14...eccdd38

marcelofabri · 2016-12-15T19:35:24Z

CHANGELOG.md

@@ -10,6 +10,9 @@

 ##### Enhancements

+* Add a rule that enforces alphabetical sorting of imports.


missing the 2 trailing whitespaces

marcelofabri · 2016-12-15T19:35:33Z

CHANGELOG.md

@@ -10,6 +10,9 @@

 ##### Enhancements

+* Add a rule that enforces alphabetical sorting of imports.
+  [Scott Berrevoets](https://github.com/sberrevoets)


can you add the issue number too?

marcelofabri · 2016-12-15T19:37:20Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+            return [
+                    StyleViolation(ruleDescription: type(of: self).description,
+                                  severity: configuration.severity,
+                                  location: Location(file: file, byteOffset: lastImportLocation))


this should use characterOffset, because the range is coming from a regex match. When the offset comes from SourceKit (for example, from a SyntaxToken) you should use byteOffset

I used https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/EmptyCountRule.swift#L39 for inspiration, where this also uses byteOffset. Is that different somehow or should it be changed there as well?

I think it should be changed there as well, but let @jpsim confirm that. I could be wrong about this 😅

ghost · 2016-12-16T07:05:06Z

@sberrevoets 🤔 Maybe it could treat comment lines as separators to allow you to group the import statements and then alphabetise within each group? This is the most common use case I've seen for not having them all alphabetised at the file level. Or maybe that's a configurable option?

sberrevoets · 2016-12-16T10:48:25Z

To be honest, I've never seen that style. I've seen groupings within imports before (by having a blank line between groups), but that was mostly in the Objective-C world where every individual file had to be imported, and still at the top of the file.

If we were to go down that path I think I'd use a blank line as group separator, or even any line that doesn't start with import.

jpsim · 2016-12-16T21:53:44Z

It's hard to determine exactly which import is in violation if the list is not sorted. For example, in this list:
import ZZZ
import AAA
import BBB
import CCC

Why not just report violations for items that aren't "greater" than the ones before it?

It would generate these violations:

import ZZZ
↓import AAA
import BBB
import CCC

import AAA
import BBB
import ZZZ
↓import CCC

import BBB
↓import AAA
import ZZZ
↓import CCC

Which IMHO is generally more useful than the current algorithm.

sberrevoets · 2016-12-17T09:51:16Z

That would work, though in your first example marking AAA would still be a little weird since really it's ZZZ that's out of order. But I agree that it would still be more useful than the current implementation, so I'll change the code to behave like that.

sberrevoets · 2016-12-17T12:09:46Z

@jpsim Updated to mark violating lines the way you suggested.

@marcelofabri I noticed you changed the example I based mine on so I changed this PR to use characterOffset as well.

marcelofabri · 2016-12-17T12:53:01Z

@sberrevoets Thanks for updating this. I've also opened #997 to prevent us to do merge something like that again 😅

jpsim · 2016-12-17T20:16:52Z

CHANGELOG.md

@@ -10,6 +10,10 @@

 ##### Enhancements

+* Add a rule that enforces alphabetical sorting of imports.  
+  [Scott Berrevoets](https://github.com/sberrevoets)
+  [#991](https://github.com/realm/SwiftLint/pull/991)


reference the issue number this solves (#900): https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

jpsim · 2016-12-17T20:19:14Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+
+    public func validateFile(_ file: File) -> [StyleViolation] {
+        let pattern = "import\\s+(\\w+)"
+        let excludingKinds = SyntaxKind.commentAndStringKinds()


instead of excluding comments and strings, you should be matching a single keyword followed by a single identifier, as highlighted by getting the syntax information from imports:

$ sourcekitten syntax --text "import AAA" [ { "type" : "source.lang.swift.syntaxtype.keyword", "offset" : 0, "length" : 6 }, { "type" : "source.lang.swift.syntaxtype.identifier", "offset" : 7, "length" : 3 } ]

There's a similar check in https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/AttributesRule.swift#L67-L72, it you need inspiration ;)

jpsim · 2016-12-17T20:20:16Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+            "import AAA\nimport BBB\nimport CCC\nimport DDD"
+        ],
+        triggeringExamples: [
+            "import AAA\nimport ZZZ\nimport BBB\nimport CCC",


You should be inserting the ↓ character where you expect the violation to be reported.

marcelofabri · 2016-12-18T00:01:52Z

Tests/SwiftLintFrameworkTests/RulesTests.swift

@@ -263,6 +263,10 @@ class RulesTests: XCTestCase {
        verifyRule(ReturnArrowWhitespaceRule.description)
    }

+    func testSortedImports() {


you should add this to allTests in the end of this file to make it run on Linux as well

marcelofabri · 2016-12-21T14:13:35Z

CHANGELOG.md

@@ -13,6 +13,7 @@
 * Add a rule that enforces alphabetical sorting of imports.  
  [Scott Berrevoets](https://github.com/sberrevoets)
  [#991](https://github.com/realm/SwiftLint/pull/991)


Only the issue should be listed here, not the pull request

sberrevoets · 2016-12-21T14:19:04Z

@jpsim @marcelofabri Thanks for the patience and guiding me through! I've incorporated your comments in the latest updates.

marcelofabri · 2016-12-21T15:01:33Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+    )
+
+    public func validateFile(_ file: File) -> [StyleViolation] {
+        let pattern = "import (\\w+)"


Sorry to nitpick, but could you change the regex to import\\s+\\w+, since the rule shouldn't assume that there's only one space? Also, the capture group doesn't seem to be used, so it can be remove to make the regex faster.

This would probably make the offset calculation harder, but IMO we can trigger in the import itself (i.e. the regex match itself). What do you think?

Actually, the current implementation would give incorrect results if you had one import with multiple spaces and another one with just one, since the whole match is used for comparison.

The previous implementation allowed for any number of spaces, but for exactly the reason you pointed out I decided to stick with 1 since I don't see why someone would (intentionally) use more than 1 space.

However, I like the idea of the module name triggering the warning, so I've pushed a commit that adds some complexity in finding the module name. It assumes the last character before it is a space, which means import\nMyModule will fail, but IMO that's an acceptable compromise. If you think we should just trigger on import I can live with that also.

I've removed the capture group from the regex as well.

marcelofabri · 2016-12-21T19:17:35Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+                return nil
+            }
+
+            let characterOffset = range.location + moduleCharacterOffset


shouldn't this use moduleStartIndex as well? Then you can remove moduleCharacterOffset as well

Yep, totally right!

marcelofabri

Thanks for doing this 👏

jpsim · 2016-12-21T19:45:14Z

CHANGELOG.md

@@ -10,6 +10,10 @@

 ##### Enhancements

+* Add a rule that enforces alphabetical sorting of imports.  


this is now in the wrong changelog section due to 0.14 being released.

jpsim · 2016-12-22T00:05:14Z

Source/SwiftLintFramework/Rules/SortedImportsRule.swift

+    )
+
+    public func validateFile(_ file: File) -> [StyleViolation] {
+        let pattern = "import\\s+\\w+"


What do you think of this implementation instead?

let importRanges = file.matchPattern("import\\s+\\w+", withSyntaxKinds: [.keyword, .identifier]) let nsstringContents = file.contents.bridge() let importLength = 6 let modulesAndOffsets: [(String, Int)] = importRanges.map { range in let rangeAfterImport = NSRange(location: range.location + importLength, length: range.length - importLength) let module = nsstringContents.substring(with: rangeAfterImport) .trimmingCharacters(in: .whitespacesAndNewlines) let offset = NSMaxRange(range) - module.bridge().length return (module, offset) } let modulePairs = zip(modulesAndOffsets, modulesAndOffsets.dropFirst()) let violatingOffsets = modulePairs.flatMap { previous, current in return current < previous ? current.1 : nil } return violatingOffsets.map { StyleViolation(ruleDescription: type(of: self).description, severity: configuration.severity, location: Location(file: file, characterOffset: $0)) }

It avoids the flatMap/previousMatch side-effect mutation, avoids some of the bridging operations, leverages the existing matchPattern(_:withSyntaxKind:) method and avoids the cutsey use of defer.

To be completely honest, for me personally the hit on readability wouldn't be worth the benefits in this case:

Not needing previousMatch is definitely nice, no argument there

The bridging is reduced to once for every import - which is a decent win but probably not very noticeable considering the matches are short and there aren't many of them

I wasn't aware of matchPattern(_:withSyntaxKind:), but it appears that would even work with the current implementation (which I agree would definitely be nicer!)

I personally don't have a huge problem with the defer, but I can see how that can require a double-take

That said, I'm totally onboard with following the general coding practices of the existing codebase (which your proposal seems more in line with) so I've pushed another commit with that implementation.

I actually agree with all your points, I didn't share that sample thinking it was necessarily superior, just had some different ideas and I wanted to spark a discussion.

…be sorted

marcelofabri · 2016-12-26T20:29:22Z

Merged in #1071. Thanks again for doing this, @sberrevoets! 💯

marcelofabri reviewed Dec 15, 2016

View reviewed changes

jpsim requested changes Dec 17, 2016

View reviewed changes

marcelofabri reviewed Dec 18, 2016

View reviewed changes

marcelofabri reviewed Dec 21, 2016

View reviewed changes

marcelofabri approved these changes Dec 21, 2016

View reviewed changes

jpsim requested changes Dec 22, 2016

View reviewed changes

sberrevoets added 7 commits December 23, 2016 00:03

Add rule requiring imports be sorted

706ef20

fixup! Add rule requiring imports be sorted

8df7d26

fixup! fixup! Add rule requiring imports be sorted

cb82773

fixup! fixup! fixup! Add rule requiring imports be sorted

af792c2

fixup! fixup! fixup! fixup! Add rule requiring imports be sorted

7f9e0eb

fixup! fixup! fixup! fixup! fixup! Add rule requiring imports be sorted

8c425c5

fixup! fixup! fixup! fixup! fixup! fixup! Add rule requiring imports …

ed6475f

…be sorted

marcelofabri mentioned this pull request Dec 26, 2016

Sorted imports #1071

Merged

marcelofabri closed this Dec 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule requiring imports be sorted #991

Add rule requiring imports be sorted #991

sberrevoets commented Dec 15, 2016 •

edited

Loading

codecov-io commented Dec 15, 2016 •

edited

Loading

marcelofabri Dec 15, 2016

marcelofabri Dec 15, 2016

marcelofabri Dec 15, 2016

sberrevoets Dec 15, 2016

marcelofabri Dec 15, 2016

ghost commented Dec 16, 2016

sberrevoets commented Dec 16, 2016

jpsim commented Dec 16, 2016

sberrevoets commented Dec 17, 2016

sberrevoets commented Dec 17, 2016

marcelofabri commented Dec 17, 2016

jpsim Dec 17, 2016

jpsim Dec 17, 2016

marcelofabri Dec 17, 2016

jpsim Dec 17, 2016

marcelofabri Dec 18, 2016

marcelofabri Dec 21, 2016

sberrevoets commented Dec 21, 2016

marcelofabri Dec 21, 2016

marcelofabri Dec 21, 2016 •

edited

Loading

sberrevoets Dec 21, 2016 •

edited

Loading

marcelofabri Dec 21, 2016

sberrevoets Dec 21, 2016

marcelofabri left a comment

jpsim Dec 21, 2016

jpsim Dec 22, 2016

sberrevoets Dec 22, 2016

jpsim Dec 23, 2016

marcelofabri commented Dec 26, 2016

		@@ -10,6 +10,9 @@

		##### Enhancements

		* Add a rule that enforces alphabetical sorting of imports.

		@@ -10,6 +10,10 @@

		##### Enhancements

		* Add a rule that enforces alphabetical sorting of imports.

Add rule requiring imports be sorted #991

Add rule requiring imports be sorted #991

Conversation

sberrevoets commented Dec 15, 2016 • edited Loading

codecov-io commented Dec 15, 2016 • edited Loading

Current coverage is 82.53% (diff: 92.85%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 16, 2016

sberrevoets commented Dec 16, 2016

jpsim commented Dec 16, 2016

sberrevoets commented Dec 17, 2016

sberrevoets commented Dec 17, 2016

marcelofabri commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sberrevoets commented Dec 21, 2016

Choose a reason for hiding this comment

marcelofabri Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

sberrevoets Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Dec 26, 2016

sberrevoets commented Dec 15, 2016 •

edited

Loading

codecov-io commented Dec 15, 2016 •

edited

Loading

marcelofabri Dec 21, 2016 •

edited

Loading

sberrevoets Dec 21, 2016 •

edited

Loading