-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Recursive Configs #301
Recursive Configs #301
Changes from all commits
12a4316
8147a3e
c8fc563
cded5c9
0bf09ef
d8c418e
a38abbb
27ccc06
aa5dd2c
6e336a5
599f3f1
17cecab
44cb7a5
277d693
a974317
87afad3
f49003e
a523cf9
2ef4701
37b462d
089d89e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
+2 −2 | CHANGELOG.md | |
+1 −1 | Source/SourceKittenFramework/Info.plist | |
+37 −62 | Source/SourceKittenFramework/String+SourceKitten.swift | |
+1 −1 | Source/sourcekitten/Info.plist |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,4 +69,16 @@ extension String { | |
} | ||
return nil | ||
} | ||
|
||
var stringByDeletingLastPathComponent: String { | ||
return (self as NSString).stringByDeletingLastPathComponent | ||
} | ||
|
||
func stringByAppendingPathComponent(pathComponent: String) -> String { | ||
return (self as NSString).stringByAppendingPathComponent(pathComponent) | ||
} | ||
|
||
public func absolutePathStandardized() -> String { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible that we never actually need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Certainly possible. I didn't do much testing other than realizing |
||
return (self.absolutePathRepresentation() as NSString).stringByStandardizingPath | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,15 @@ extension Yaml { | |
} | ||
} | ||
|
||
public struct Configuration { | ||
public struct Configuration: Equatable { | ||
public let disabledRules: [String] // disabled_rules | ||
public let included: [String] // included | ||
public let excluded: [String] // excluded | ||
public let reporter: String // reporter (xcode, json, csv, checkstyle) | ||
public let rules: [Rule] | ||
public let useNestedConfigs: Bool // process nested configs, will default to false | ||
public var rootPath: String? // the root path of the lint to search for nested configs | ||
private var configPath: String? // if successfully load from a path | ||
|
||
public var reporterFromString: Reporter.Type { | ||
switch reporter { | ||
|
@@ -47,10 +50,12 @@ public struct Configuration { | |
included: [String] = [], | ||
excluded: [String] = [], | ||
reporter: String = "xcode", | ||
rules: [Rule] = Configuration.rulesFromYAML()) { | ||
rules: [Rule] = Configuration.rulesFromYAML(), | ||
useNestedConfigs: Bool = false) { | ||
self.included = included | ||
self.excluded = excluded | ||
self.reporter = reporter | ||
self.useNestedConfigs = useNestedConfigs | ||
|
||
// Validate that all rule identifiers map to a defined rule | ||
|
||
|
@@ -89,23 +94,36 @@ public struct Configuration { | |
} | ||
|
||
public init?(yaml: String) { | ||
let yamlResult = Yaml.load(yaml) | ||
guard let yamlConfig = yamlResult.value else { | ||
if let error = yamlResult.error { | ||
queuedPrint(error) | ||
} | ||
guard let yamlConfig = Configuration.loadYaml(yaml) else { | ||
return nil | ||
} | ||
self.init(yamlConfig: yamlConfig) | ||
} | ||
|
||
private init?(yamlConfig: Yaml) { | ||
self.init( | ||
disabledRules: yamlConfig["disabled_rules"].arrayOfStrings ?? [], | ||
included: yamlConfig["included"].arrayOfStrings ?? [], | ||
excluded: yamlConfig["excluded"].arrayOfStrings ?? [], | ||
reporter: yamlConfig["reporter"].string ?? XcodeReporter.identifier, | ||
rules: Configuration.rulesFromYAML(yamlConfig) | ||
rules: Configuration.rulesFromYAML(yamlConfig), | ||
useNestedConfigs: yamlConfig["use_nested_configs"].bool ?? false | ||
) | ||
} | ||
|
||
public init(path: String = ".swiftlint.yml", optional: Bool = true) { | ||
private static func loadYaml(yaml: String) -> Yaml? { | ||
let yamlResult = Yaml.load(yaml) | ||
if let yamlConfig = yamlResult.value { | ||
return yamlConfig | ||
} else { | ||
if let error = yamlResult.error { | ||
queuedPrint(error) | ||
} | ||
return nil | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^^ This is the refactored init code. See this commit for the refactor in it's entirety. |
||
public init(path: String = ".swiftlint.yml", optional: Bool = true, silent: Bool = false) { | ||
let fullPath = (path as NSString).absolutePathRepresentation() | ||
let failIfRequired = { | ||
if !optional { fatalError("Could not read configuration file at path '\(fullPath)'") } | ||
|
@@ -118,9 +136,12 @@ public struct Configuration { | |
do { | ||
let yamlContents = try NSString(contentsOfFile: fullPath, | ||
encoding: NSUTF8StringEncoding) as String | ||
if let _ = Configuration(yaml: yamlContents) { | ||
queuedPrintError("Loading configuration from '\(path)'") | ||
self.init(yaml: yamlContents)! | ||
if let yamlConfig = Configuration.loadYaml(yamlContents) { | ||
if !silent { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 much better IMO |
||
queuedPrintError("Loading configuration from '\(path)'") | ||
} | ||
self.init(yamlConfig: yamlConfig)! | ||
configPath = fullPath | ||
return | ||
} else { | ||
failIfRequired() | ||
|
@@ -188,4 +209,54 @@ public struct Configuration { | |
let allPaths = self.lintablePathsForPath(path) | ||
return allPaths.flatMap { File(path: $0) } | ||
} | ||
|
||
public func configForFile(file: File) -> Configuration { | ||
if useNestedConfigs, | ||
let containingDir = file.path?.stringByDeletingLastPathComponent { | ||
return configForPath(containingDir) | ||
} | ||
return self | ||
} | ||
} | ||
|
||
// MARK: - Nested Configurations Extension | ||
|
||
public extension Configuration { | ||
func configForPath(path: String) -> Configuration { | ||
let configSearchPath = path.stringByAppendingPathComponent(".swiftlint.yml") | ||
|
||
// If a config exists and it isn't us, load and merge the configs | ||
if configSearchPath != configPath && | ||
NSFileManager.defaultManager().fileExistsAtPath(configSearchPath) { | ||
return merge(Configuration(path: configSearchPath, optional: false, silent: true)) | ||
} | ||
|
||
// If we are not at the root path, continue down the tree | ||
if path != rootPath { | ||
return configForPath(path.stringByDeletingLastPathComponent) | ||
} | ||
|
||
// If nothing else, return self | ||
return self | ||
} | ||
|
||
// Currently merge simply overrides the current configuration with the new configuration. | ||
// This requires that all config files be fully specified. In the future this will be changed | ||
// to do a more intelligent merge allowing for partial nested configs. | ||
func merge(config: Configuration) -> Configuration { | ||
return config | ||
} | ||
} | ||
|
||
// Mark - == Implementation | ||
|
||
public func == (lhs: Configuration, rhs: Configuration) -> Bool { | ||
return (lhs.disabledRules == rhs.disabledRules) && | ||
(lhs.excluded == rhs.excluded) && | ||
(lhs.included == rhs.included) && | ||
(lhs.reporter == rhs.reporter) && | ||
(lhs.useNestedConfigs == rhs.useNestedConfigs) && | ||
(lhs.configPath == rhs.configPath) && | ||
(lhs.rootPath == lhs.rootPath) && | ||
(lhs.rules == rhs.rules) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,12 +13,35 @@ public protocol Rule { | |
func validateFile(file: File) -> [StyleViolation] | ||
} | ||
|
||
extension Rule { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not possible to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well it would be possible. The problem is that making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping as-is is fine then. |
||
func isEqualTo(rule: Rule) -> Bool { | ||
return self.dynamicType.description == rule.dynamicType.description | ||
} | ||
} | ||
|
||
public protocol ParameterizedRule: Rule { | ||
typealias ParameterType | ||
typealias ParameterType: Equatable | ||
init(parameters: [RuleParameter<ParameterType>]) | ||
var parameters: [RuleParameter<ParameterType>] { get } | ||
} | ||
|
||
extension ParameterizedRule { | ||
func isEqualTo(rule: Self) -> Bool { | ||
return (self.dynamicType.description == rule.dynamicType.description) && | ||
(self.parameters == rule.parameters) | ||
} | ||
} | ||
|
||
public protocol CorrectableRule: Rule { | ||
func correctFile(file: File) -> [Correction] | ||
} | ||
|
||
// MARK: - == Implementations | ||
|
||
func == (lhs: [Rule], rhs: [Rule]) -> Bool { | ||
if lhs.count == rhs.count { | ||
return zip(lhs, rhs).map { $0.isEqualTo($1) }.reduce(true) { $0 && $1 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can avoid creating the intermediate closures here: zip(lhs, rhs).map(Rule.isEqualTo).reduce(true, &&) |
||
} | ||
|
||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,8 @@ | |
// Copyright © 2015 Realm. All rights reserved. | ||
// | ||
|
||
import SwiftLintFramework | ||
@testable import SwiftLintFramework | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's probably ok. I get slightly nervous with equality stuff because it's easy for someone to add a member to the respective type without including it in the member-wise check. You can somewhat mitigate this by using Swift's limited reflection to write a test to assert that the number of members hasn't changed. But I often just keep this stuff out of public API until the data structures have stabilized or it's otherwise needed. Just let me know what you'd prefer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping as-is is fine. |
||
import SourceKittenFramework | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this import used anywhere? I can't see. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, 👍 then |
||
import XCTest | ||
|
||
class ConfigurationTests: XCTestCase { | ||
|
@@ -89,4 +90,105 @@ class ConfigurationTests: XCTestCase { | |
let paths = configuration.lintablePathsForPath("", fileManager: TestFileManager()) | ||
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"], paths) | ||
} | ||
|
||
// MARK: - Testing Configuration Equality | ||
|
||
private var projectMockConfig0: Configuration { | ||
var config = Configuration(path: projectMockYAML0, optional: false, silent: true) | ||
config.rootPath = projectMockPathLevel0 | ||
return config | ||
} | ||
|
||
private var projectMockConfig2: Configuration { | ||
return Configuration(path: projectMockYAML2, optional: false, silent: true) | ||
} | ||
|
||
func testIsEqualTo() { | ||
XCTAssertEqual(projectMockConfig0, projectMockConfig0) | ||
} | ||
|
||
func testIsNotEqualTo() { | ||
XCTAssertNotEqual(projectMockConfig0, projectMockConfig2) | ||
} | ||
|
||
// MARK: - Testing Nested Configurations | ||
|
||
func testMerge() { | ||
XCTAssertEqual(projectMockConfig0.merge(projectMockConfig2), projectMockConfig2) | ||
} | ||
|
||
func testLevel0() { | ||
XCTAssertEqual(projectMockConfig0.configForFile(File(path: projectMockSwift0)!), | ||
projectMockConfig0) | ||
} | ||
|
||
func testLevel1() { | ||
XCTAssertEqual(projectMockConfig0.configForFile(File(path: projectMockSwift1)!), | ||
projectMockConfig0) | ||
} | ||
|
||
func testLevel2() { | ||
XCTAssertEqual(projectMockConfig0.configForFile(File(path: projectMockSwift2)!), | ||
projectMockConfig0.merge(projectMockConfig2)) | ||
} | ||
|
||
func testLevel3() { | ||
XCTAssertEqual(projectMockConfig0.configForFile(File(path: projectMockSwift3)!), | ||
projectMockConfig0.merge(projectMockConfig2)) | ||
} | ||
|
||
func testDoNotUseNestedConfigs() { | ||
var config = Configuration(yaml: "use_nested_configs: false\n")! | ||
config.rootPath = projectMockPathLevel0 | ||
XCTAssertEqual(config.configForFile(File(path: projectMockSwift3)!), | ||
config) | ||
} | ||
} | ||
|
||
// MARK: - ProjectMock Paths | ||
|
||
extension XCTestCase { | ||
var bundlePath: String { | ||
return NSBundle(forClass: self.dynamicType).resourcePath! | ||
} | ||
|
||
var projectMockPathLevel0: String { | ||
return bundlePath.stringByAppendingPathComponent("ProjectMock") | ||
} | ||
|
||
var projectMockPathLevel1: String { | ||
return projectMockPathLevel0.stringByAppendingPathComponent("Level1") | ||
} | ||
|
||
var projectMockPathLevel2: String { | ||
return projectMockPathLevel1.stringByAppendingPathComponent("Level2") | ||
} | ||
|
||
var projectMockPathLevel3: String { | ||
return projectMockPathLevel2.stringByAppendingPathComponent("Level3") | ||
} | ||
|
||
var projectMockYAML0: String { | ||
return projectMockPathLevel0.stringByAppendingPathComponent(".swiftlint.yml") | ||
} | ||
|
||
var projectMockYAML2: String { | ||
return projectMockPathLevel2.stringByAppendingPathComponent(".swiftlint.yml") | ||
} | ||
|
||
var projectMockSwift0: String { | ||
return projectMockPathLevel0.stringByAppendingPathComponent("Level0.swift") | ||
} | ||
|
||
var projectMockSwift1: String { | ||
return projectMockPathLevel1.stringByAppendingPathComponent("Level1.swift") | ||
} | ||
|
||
var projectMockSwift2: String { | ||
return projectMockPathLevel2.stringByAppendingPathComponent("Level2.swift") | ||
} | ||
|
||
var projectMockSwift3: String { | ||
return projectMockPathLevel3.stringByAppendingPathComponent("Level3.swift") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
disabled_rules: | ||
- force_cast | ||
included: | ||
- "everything" | ||
exluded: | ||
- "the place where i committed many coding sins" | ||
line_length: 10000000000 | ||
reporter: "json" | ||
use_nested_configs: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
// This is just a mock Swift file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For methods on NSString, I prefer not masking them behind a
Swift.String
interface, since that hides the fact that the string has to be cast to NSString. Especially since SwiftLint (and SourceKitten) make heavy use of NSString in its implementation, you then end up casting an NSString into aSwift.String
to use these methods, which then cast it back to an NSString.For these reasons, I prefer casting at the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That motivation makes a lot of sense to me. My thinking was mostly DRY, and how it would make it trivial to swap this out project-wide with a Swift
String
native approach if desired. But I'd be happy to revert.