-
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
Configuration Structs #391
Changes from all commits
d300eec
21abbd3
ccbcb53
1a2bb1b
619a345
d2ec3fb
6522add
b92c241
4ad287d
6d38573
0883f56
0b61c63
05a65af
873edb7
ddcde17
ee3fe42
84aa479
0691b79
d2c697d
810f813
d137dcd
bf5a569
20e8038
53e3057
a64f9ed
421aa6e
3230c64
60546e4
1f13e16
cd5f457
8ff1562
989bac7
7af4756
f722bcf
943ed30
885b47d
284d4e1
123d06e
6132d5d
42a6e9f
9f20fc2
95a7352
e2265cf
6107822
b145bf1
87c5342
d1574bb
53218ec
6718de4
44ff8ef
12aba5f
75f2068
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// | ||
// ConfigurationError.swift | ||
// SwiftLint | ||
// | ||
// Created by Scott Hoyt on 1/19/16. | ||
// Copyright © 2016 Realm. All rights reserved. | ||
// | ||
|
||
import Foundation | ||
|
||
public enum ConfigurationError: ErrorType { | ||
case UnknownConfiguration | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// | ||
// RuleConfig.swift | ||
// SwiftLint | ||
// | ||
// Created by Scott Hoyt on 1/19/16. | ||
// Copyright © 2016 Realm. All rights reserved. | ||
// | ||
|
||
import Foundation | ||
|
||
public protocol RuleConfig { | ||
mutating func setConfig(config: AnyObject) throws | ||
func isEqualTo(ruleConfig: RuleConfig) -> Bool | ||
} | ||
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 can use: extension RuleConfig where Self: Equatable {
public func isEqualTo(ruleConfig: RuleConfig) -> Bool {
if let ruleConfig = ruleConfig as? Self {
return self == ruleConfig
}
return false
}
} 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. Great add. 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. Done! 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. 👍 |
||
|
||
extension RuleConfig where Self: Equatable { | ||
public func isEqualTo(ruleConfig: RuleConfig) -> Bool { | ||
if let ruleConfig = ruleConfig as? Self { | ||
return self == ruleConfig | ||
} | ||
return false | ||
} | ||
} |
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.
Why introduce this if there's only one member, and it's never checked?
ConfigurableRuleType.init(config:)
is only ever used withtry?
, which type-erases the error in a sense.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.
This was something I thought about for a bit. Basically what was the best way to support configuration structures failing and provide optional configuration values functionality. To use the failable initializers approach that was previously used in
ConfigurableRule
would have required each rule to have its ownRuleConfig
type because you could only declare the default values in the structure. The second approach was to split initialization with default values and configuration with optional values intoinit
andsetConfig
. Since we needed a way to indicate failure insetConfig
, we could either throw or return some sort of success/failure flag (e.g. Bool,Result
, etc.). Throwing felt the most Swifty to me, and I needed something to throw, so that's how this came to be.There's certainly a case to be made that if we use this approach, we should introduce some finer-grain errors, possibly merge this with YamlParsingError, and probably change
Configuration.init?
toConfiguration.init() throws
. That's all possible in this PR, but in my head I was scoping it to the rework that needs to be done on logging and errors.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 the time being, I made this marginally more useful by not squashing the error in
Configuration.rulesFromDict
, though nothing is really being done with it. Let me know if you want to combine this withYamlParserError
. I like that we have one less error type with that approach. But I don't like that aConfigProviderRule
would even know about something that knows about theYaml
subsystem.