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.

Sign up for GitHub

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

Disable SeverityLevelsConfig error ruleParam if only warning is set #431

Closed
wants to merge 1 commit into from
Closed

Disable SeverityLevelsConfig error ruleParam if only warning is set #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

##### Breaking

* None.
* Setting only warning on `SeverityLevelsConfig` now
disables error ruleParameter.
[Robin Kunde](https://github.com/robinkunde)
[#409](https://github.com/realm/SwiftLint/issues/409)

##### Enhancements

Expand Down
4 changes: 2 additions & 2 deletions Source/SwiftLintFramework/Rules/RuleConfigs/NameConfig.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ public struct NameConfig: RuleConfig, Equatable {
var excluded: Set<String>

var minLengthThreshold: Int {
return max(minLength.warning, minLength.error)
return max(minLength.warning, minLength.error ?? minLength.warning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just be return max(minLength.warning, minLength.error ?? minLength.warning)

}

var maxLengthThreshold: Int {
return min(maxLength.warning, maxLength.error)
return min(maxLength.warning, maxLength.error ?? maxLength.warning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just be return min(maxLength.warning, maxLength.error ?? maxLength.warning)

}

public init(minLengthWarning: Int,
Expand Down
15 changes: 8 additions & 7 deletions Source/SwiftLintFramework/Rules/RuleConfigs/SeverityLevelsConfig.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,25 @@ import Foundation

public struct SeverityLevelsConfig: RuleConfig, Equatable {
var warning: Int
var error: Int
var error: Int?

var params: [RuleParameter<Int>] {
return [RuleParameter(severity: .Error, value: error),
if let error = error {
return [RuleParameter(severity: .Error, value: error),
RuleParameter(severity: .Warning, value: warning)]
}
return [RuleParameter(severity: .Warning, value: warning)]
}

mutating public func setConfig(config: AnyObject) throws {
if let config = [Int].arrayOf(config) where !config.isEmpty {
warning = config[0]
if config.count > 1 {
error = config[1]
}
error = (config.count > 1) ? config[1] : nil
} else if let config = config as? [String: AnyObject] {
if let warningNumber = config["warning"] as? Int {
warning = warningNumber
}
if let errorNumber = config["error"] as? Int {
error = config["error"] as? Int
} else if let errorNumber = config["error"] as? Int {
error = errorNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this can just be replaced with:

if let warningNumber = config["warning"] as? Int {
    warning = warningNumber
}
error = config["error"] as? Int

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's what I tried to convey in my original message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

An empty dictionary or dictionaries containing keys other than error and warning are configuration errors. I added validation for this in #438.

}
} else {
Expand Down
37 changes: 36 additions & 1 deletion Source/SwiftLintFrameworkTests/RuleConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//

import XCTest
import SwiftLintFramework
@testable import SwiftLintFramework
import SourceKittenFramework

class RuleConfigurationsTests: XCTestCase {
Expand Down Expand Up @@ -44,6 +44,30 @@ class RuleConfigurationsTests: XCTestCase {
}
}

func testNameConfigMinLengthThreshold() {
var nameConfig = NameConfig(minLengthWarning: 7,
minLengthError: 17,
maxLengthWarning: 0,
maxLengthError: 0,
excluded: [])
XCTAssertEqual(nameConfig.minLengthThreshold, 17)

nameConfig.minLength.error = nil
XCTAssertEqual(nameConfig.minLengthThreshold, 7)
}

func testNameConfigMaxLengthThreshold() {
var nameConfig = NameConfig(minLengthWarning: 0,
minLengthError: 0,
maxLengthWarning: 17,
maxLengthError: 7,
excluded: [])
XCTAssertEqual(nameConfig.maxLengthThreshold, 7)

nameConfig.maxLength.error = nil
XCTAssertEqual(nameConfig.maxLengthThreshold, 17)
}

func testSeverityConfigFromString() {
let config = "Warning"
let comp = SeverityConfig(.Warning)
Expand Down Expand Up @@ -76,6 +100,17 @@ class RuleConfigurationsTests: XCTestCase {
}
}

func testSeverityLevelConfigParams() {
let severityConfig = SeverityLevelsConfig(warning: 17, error: 7)
XCTAssertEqual(severityConfig.params, [RuleParameter(severity: .Error, value: 7),
RuleParameter(severity: .Warning, value: 17)])
}

func testSeverityLevelConfigPartialParams() {
let severityConfig = SeverityLevelsConfig(warning: 17, error: nil)
XCTAssertEqual(severityConfig.params, [RuleParameter(severity: .Warning, value: 17)])
}

func testRegexConfigThrows() {
let config = 17
var regexConfig = RegexConfig(identifier: "")
Expand Down
39 changes: 33 additions & 6 deletions Source/SwiftLintFrameworkTests/RuleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import SourceKittenFramework
struct RuleWithLevelsMock: ConfigProviderRule {
var config = SeverityLevelsConfig(warning: 2, error: 3)

static let description = RuleDescription(identifier: "violation_level_mock",
static let description = RuleDescription(identifier: "severity_level_mock",
name: "",
description: "")
func validateFile(file: File) -> [StyleViolation] { return [] }
Expand Down Expand Up @@ -58,7 +58,7 @@ class RuleTests: XCTestCase {
XCTAssertFalse([RuleMock1(), RuleMock2()] == [RuleMock1()])
}

func testViolationLevelRuleInitsWithConfigDictionary() {
func testSeverityLevelRuleInitsWithConfigDictionary() {
let config = ["warning": 17, "error": 7]
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
Expand All @@ -67,7 +67,24 @@ class RuleTests: XCTestCase {
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testViolationLevelRuleInitsWithConfigArray() {
func testSeverityLevelRuleInitsWithWarningOnlyConfigDictionary() {
let config = ["warning": 17]
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
comp.config.warning = 17
comp.config.error = nil
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testSeverityLevelRuleInitsWithErrorOnlyConfigDictionary() {
let config = ["error": 17]
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
comp.config.error = 17
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testSeverityLevelRuleInitsWithConfigArray() {
let config = [17, 7] as AnyObject
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
Expand All @@ -76,21 +93,31 @@ class RuleTests: XCTestCase {
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testViolationLevelRuleInitsWithLiteral() {
func testSeverityLevelRuleInitsWithSingleValueConfigArray() {
let config = [17] as AnyObject
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
comp.config.warning = 17
comp.config.error = nil
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testSeverityLevelRuleInitsWithLiteral() {
let config = 17 as AnyObject
let rule = try? RuleWithLevelsMock(config: config)
var comp = RuleWithLevelsMock()
comp.config.warning = 17
comp.config.error = nil
XCTAssertEqual(rule?.isEqualTo(comp), true)
}

func testViolationLevelRuleNotEqual() {
func testSeverityLevelRuleNotEqual() {
let config = 17 as AnyObject
let rule = try? RuleWithLevelsMock(config: config)
XCTAssertEqual(rule?.isEqualTo(RuleWithLevelsMock()), false)
}

func testDifferentViolationLevelRulesNotEqual() {
func testDifferentSeverityLevelRulesNotEqual() {
XCTAssertFalse(RuleWithLevelsMock().isEqualTo(RuleWithLevelsMock2()))
}
}