Disable SeverityLevelsConfig error ruleParam if only warning is set #431
Conversation
| @@ -58,7 +58,7 @@ class RuleTests: XCTestCase { | |||
| XCTAssertFalse([RuleMock1(), RuleMock2()] == [RuleMock1()]) | |||
| } | |||
|
|
|||
| func testViolationLevelRuleInitsWithConfigDictionary() { | |||
| func testSecurityLevelRuleInitsWithConfigDictionary() { | |||
There was a problem hiding this comment.
This should be testSeverityLevelRuleInitsWithConfigDictionary. All the other tests changed in this file now also say "Security" where they should say "Severity".
|
Thanks for the PR, @robinkunde! This is fine, but having |
|
@jpsim That's much Swiftier solution! I've amended the PR accordingly. |
| @@ -14,11 +14,19 @@ public struct NameConfig: RuleConfig, Equatable { | |||
| var excluded: Set<String> | |||
|
|
|||
| var minLengthThreshold: Int { | |||
| return max(minLength.warning, minLength.error) | |||
There was a problem hiding this comment.
This can just be return max(minLength.warning, minLength.error ?? minLength.warning)
|
Sorry for all the nitpicky comments, @robinkunde, this is definitely going in the right direction. Please also add a changelog entry following our guidelines. This is technically a breaking change. |
|
No worries, I appreciate the pointers. I've updated the commit. |
| } | ||
| if let errorNumber = config["error"] as? Int { | ||
| error = config["error"] as? Int | ||
| } else if let errorNumber = config["error"] as? Int { | ||
| error = errorNumber |
There was a problem hiding this comment.
I'm pretty sure this can just be replaced with:
if let warningNumber = config["warning"] as? Int {
warning = warningNumber
}
error = config["error"] as? IntWhat do you think?
There was a problem hiding this comment.
Yes, that's what I tried to convey in my original message.
There was a problem hiding this comment.
I thought about it, but it would change the behavior of the method in cases of config:
- being an empty dictionary
- containing keys besides 'warning' and 'error' (because of typos, for example)
The first one can be addressed easily enough with a where clause. The second case might be ok too, since we're documenting this as a breaking change. However, since there's no formal config validator, I wasn't quite comfortable going that route.
What do you think?
There was a problem hiding this comment.
An empty dictionary or dictionaries containing keys other than error and warning are configuration errors. I added validation for this in #438.
|
Thanks for the PR, it's looking good! |
…This prevents the potential creation of invalid configs by implicit definition in Yaml.
|
Thanks for your work here, @robinkunde. Merging in #438. |
SeverityLevelsConfig is used for both rules where the warning threshold must logically be lower than the error threshold and vice versa.
In order to prevent the generation of invalid configs via implicit definition in yaml, the error ruleParameter should be disable in such cases. For example, the README contains this code right now:
If 110 is larger than whatever default value SwiftLint define for error, this would be an invalid config.
This commit also updates the test names since ViolationLevel is no more.