Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add rule requiring imports be sorted #991

Closed
wants to merge 7 commits into from
Prev Previous commit
Next Next commit
fixup! Add rule requiring imports be sorted
sberrevoets committed Dec 22, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 8df7d2635982e96d800f0faabaf7c5b148c184d7
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -83,6 +83,7 @@
* Add a rule that enforces alphabetical sorting of imports.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

[Scott Berrevoets](https://github.com/sberrevoets)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add the issue number too?

[#991](https://github.com/realm/SwiftLint/pull/991)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

[#900](https://github.com/realm/SwiftLint/issues/900)

* Now builds and passes most tests on Linux using the Swift Package Manager with
Swift 3. This requires `libsourcekitdInProc.so` to be built and located in
36 changes: 19 additions & 17 deletions Source/SwiftLintFramework/Rules/SortedImportsRule.swift
Original file line number Diff line number Diff line change
@@ -11,6 +11,8 @@ import SourceKittenFramework
public struct SortedImportsRule: ConfigurationProviderRule, OptInRule {
public var configuration = SeverityConfiguration(.warning)

private let moduleCharacterOffset = "import ".characters.count

public init() {}

public static let description = RuleDescription(
@@ -21,31 +23,31 @@ public struct SortedImportsRule: ConfigurationProviderRule, OptInRule {
"import AAA\nimport BBB\nimport CCC\nimport DDD"
],
triggeringExamples: [
"import AAA\nimport ZZZ\nimport BBB\nimport CCC",
"import AAA\nimport ZZZ\nimport BBB\nimport CCC\nimport AAA"
"import AAA\nimport ZZZ\nimport ↓BBB\nimport CCC"
]
)

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@marcelofabri marcelofabri Dec 21, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Author

@sberrevoets sberrevoets Dec 21, 2016

Choose a reason for hiding this comment

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

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.


var previousMatch = ""
return file.matchPattern(pattern).flatMap { range, kinds in
if kinds.count != 2 || kinds[0] != .keyword || kinds[1] != .identifier {
return nil
}

let importRanges = file.matchPattern(pattern, excludingSyntaxKinds: excludingKinds)
let imports = importRanges.map { file.contents.substring($0.location, length: $0.length) }
let match = file.contents.bridge().substring(with: range)
defer { previousMatch = match }

var violations = [StyleViolation]()
for index in 0 ..< imports.count {
if index == 0 || imports[index] > imports[index - 1] {
continue
if match > previousMatch {
return nil
}

let location = Location(file: file, characterOffset: importRanges[index].location)
violations.append(
StyleViolation(ruleDescription: type(of: self).description,
severity: configuration.severity, location: location)
)
}
let characterOffset = range.location + moduleCharacterOffset
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yep, totally right!

let location = Location(file: file, characterOffset: characterOffset)
return StyleViolation(ruleDescription: type(of: self).description,
severity: configuration.severity, location: location)

return violations
}
}
}
1 change: 1 addition & 0 deletions Tests/SwiftLintFrameworkTests/RulesTests.swift
Original file line number Diff line number Diff line change
@@ -391,6 +391,7 @@ extension RulesTests {
("testRedundantNilCoalescing", testRedundantNilCoalescing),
("testRedundantStringEnumValue", testRedundantStringEnumValue),
("testReturnArrowWhitespace", testReturnArrowWhitespace),
("testSortedImports", testSortedImports),
("testStatementPosition", testStatementPosition),
("testStatementPositionUncuddled", testStatementPositionUncuddled),
("testSwitchCaseOnNewline", testSwitchCaseOnNewline),