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

Disabled rules in child configs are no longer automatically ignored if they are opt-in #4863

Merged
merged 27 commits into from
Jul 26, 2023
Merged

Disabled rules in child configs are no longer automatically ignored if they are opt-in #4863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b59ddd9
Disabled rules in child configs are no longer automatically ignored i…
mildm8nnered Apr 4, 2023
77f6eeb
Added better unit tests
mildm8nnered Apr 8, 2023
a3b4309
Refactor
mildm8nnered Apr 8, 2023
4e7bda9
Added test, but it fails
mildm8nnered Apr 8, 2023
d52cfad
Fixed only rules issue
mildm8nnered Apr 8, 2023
aa826d6
Switched to booleans
mildm8nnered Apr 9, 2023
64bac0e
Refactor
mildm8nnered Apr 9, 2023
ac90d28
naming
mildm8nnered Apr 9, 2023
fdccb54
better bitwise comparison
mildm8nnered Apr 9, 2023
477dab9
Cleanup
mildm8nnered Apr 9, 2023
ac4e184
i -> index
mildm8nnered Apr 9, 2023
a5a2555
better comparison order
mildm8nnered Apr 9, 2023
dc38254
More tweaks
mildm8nnered Apr 9, 2023
e244419
More cleanup
mildm8nnered Apr 9, 2023
5ac937a
More tests
mildm8nnered Apr 9, 2023
afcae80
more test re-arrangement
mildm8nnered Apr 10, 2023
3d900c3
Improved unit tests
mildm8nnered Jul 15, 2023
a10c76d
Switched to structs so we can test for uniqueness, as tuples cannot b…
mildm8nnered Jul 16, 2023
d83dfb2
Removed surplus test
mildm8nnered Jul 16, 2023
e996c91
added changelog entry
mildm8nnered Jul 19, 2023
949afb0
Verify rule is optin
mildm8nnered Jul 25, 2023
737a3b8
Make sure rule is not optin
mildm8nnered Jul 25, 2023
2263bbe
better messaging
mildm8nnered Jul 25, 2023
acd18a9
Correct tests, but with compiler warnings :-(
mildm8nnered Jul 25, 2023
8f8aa4a
Test rule type without hitting the warning
mildm8nnered Jul 25, 2023
0932089
Trailing newline
mildm8nnered Jul 25, 2023
f149b50
Fixed up rule type checks
mildm8nnered Jul 25, 2023
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: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@
[Martin Redington](https://github.com/mildm8nnered)
[#5120](https://github.com/realm/SwiftLint/issues/5120)

* Fix some unexpected rule enablement interactions between parent and
child configurations.
[Martin Redington](https://github.com/mildm8nnered)
[#4876](https://github.com/realm/SwiftLint/issues/4876)

## 0.52.4: Lid Switch

#### Breaking
Expand Down
7 changes: 2 additions & 5 deletions Source/SwiftLintCore/Extensions/Configuration+RulesWrapper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,7 @@ internal extension Configuration {

// Only use parent disabled / optIn if child config doesn't tell the opposite
return .default(
disabled: Set(childDisabled).union(Set(disabled.filter { !childOptIn.contains($0) }))
.filter {
!isOptInRule($0, allRulesWrapped: newAllRulesWrapped)
},
disabled: Set(childDisabled).union(Set(disabled.filter { !childOptIn.contains($0) })),
optIn: Set(childOptIn).union(Set(optIn.filter { !childDisabled.contains($0) }))
.filter {
isOptInRule($0, allRulesWrapped: newAllRulesWrapped)
Expand All @@ -262,7 +259,7 @@ internal extension Configuration {
// Allow parent only rules that weren't disabled via the child config
// & opt-ins from the child config
return .only(Set(
childOptIn + onlyRules.filter { !childDisabled.contains($0) }
childOptIn.union(onlyRules).filter { !childDisabled.contains($0) }
))

case .allEnabled:
Expand Down
116 changes: 116 additions & 0 deletions Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift
View file
Open in desktop
SimplyDanny marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,122 @@ extension ConfigurationTests {
)
}

func testParentChildOptInAndDisable() {
struct TestCase: Equatable {
let optedInInParent: Bool
let disabledInParent: Bool
let optedInInChild: Bool
let disabledInChild: Bool
let isEnabled: Bool
var message: String {
"optedInInParent = \(optedInInParent) " +
"disabledInParent = \(disabledInParent) " +
"optedInInChild = \(optedInInChild) " +
"disabledInChild = \(disabledInChild)"
}
}
let testCases: [TestCase] = [
// swiftlint:disable line_length
TestCase(optedInInParent: false, disabledInParent: false, optedInInChild: false, disabledInChild: false, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: false, optedInInChild: false, disabledInChild: false, isEnabled: true),
TestCase(optedInInParent: false, disabledInParent: true, optedInInChild: false, disabledInChild: false, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: true, optedInInChild: false, disabledInChild: false, isEnabled: false),
TestCase(optedInInParent: false, disabledInParent: false, optedInInChild: true, disabledInChild: false, isEnabled: true),
TestCase(optedInInParent: true, disabledInParent: false, optedInInChild: true, disabledInChild: false, isEnabled: true),
TestCase(optedInInParent: false, disabledInParent: true, optedInInChild: true, disabledInChild: false, isEnabled: true),
TestCase(optedInInParent: true, disabledInParent: true, optedInInChild: true, disabledInChild: false, isEnabled: true),
TestCase(optedInInParent: false, disabledInParent: false, optedInInChild: false, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: false, optedInInChild: false, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: false, disabledInParent: true, optedInInChild: false, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: true, optedInInChild: false, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: false, disabledInParent: false, optedInInChild: true, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: false, optedInInChild: true, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: false, disabledInParent: true, optedInInChild: true, disabledInChild: true, isEnabled: false),
TestCase(optedInInParent: true, disabledInParent: true, optedInInChild: true, disabledInChild: true, isEnabled: false)
// swiftlint:enable line_length
]
XCTAssertEqual(testCases.unique.count, 4 * 4)
let ruleType = ImplicitReturnRule.self
SimplyDanny marked this conversation as resolved.
Show resolved Hide resolved
XCTAssertTrue((ruleType as Any) is OptInRule.Type)
let ruleIdentifier = ruleType.description.identifier
for testCase in testCases {
let parentConfiguration = Configuration(rulesMode: .default(
disabled: testCase.disabledInParent ? [ruleIdentifier] : [],
optIn: testCase.optedInInParent ? [ruleIdentifier] : []
))
let childConfiguration = Configuration(rulesMode: .default(
disabled: testCase.disabledInChild ? [ruleIdentifier] : [],
optIn: testCase.optedInInChild ? [ruleIdentifier] : []
))
let mergedConfiguration = parentConfiguration.merged(withChild: childConfiguration, rootDirectory: "")
let isEnabled = mergedConfiguration.contains(rule: ruleType)
XCTAssertEqual(isEnabled, testCase.isEnabled, testCase.message)
}
}

func testParentChildDisableForDefaultRule() {
struct TestCase: Equatable {
let disabledInParent: Bool
let disabledInChild: Bool
let isEnabled: Bool
var message: String {
"disabledInParent = \(disabledInParent) disabledInChild = \(disabledInChild)"
}
}
let testCases: [TestCase] = [
TestCase(disabledInParent: false, disabledInChild: false, isEnabled: true),
TestCase(disabledInParent: true, disabledInChild: false, isEnabled: false),
TestCase(disabledInParent: false, disabledInChild: true, isEnabled: false),
TestCase(disabledInParent: true, disabledInChild: true, isEnabled: false)
]
XCTAssertEqual(testCases.unique.count, 2 * 2)
let ruleType = BlanketDisableCommandRule.self
XCTAssertFalse(ruleType is OptInRule.Type)
SimplyDanny marked this conversation as resolved.
Show resolved Hide resolved
let ruleIdentifier = ruleType.description.identifier
for testCase in testCases {
let parentConfiguration = Configuration(
rulesMode: .default(disabled: testCase.disabledInParent ? [ruleIdentifier] : [], optIn: [])
)
let childConfiguration = Configuration(
rulesMode: .default(disabled: testCase.disabledInChild ? [ruleIdentifier] : [], optIn: [])
)
let mergedConfiguration = parentConfiguration.merged(withChild: childConfiguration, rootDirectory: "")
let isEnabled = mergedConfiguration.contains(rule: ruleType)
XCTAssertEqual(isEnabled, testCase.isEnabled, testCase.message)
}
}

func testParentOnlyRulesAndChildOptInAndDisabled() {
struct TestCase: Equatable {
let optedInInChild: Bool
let disabledInChild: Bool
let isEnabled: Bool
var message: String {
"optedInInChild = \(optedInInChild) disabledInChild = \(disabledInChild)"
}
}
let testCases: [TestCase] = [
TestCase(optedInInChild: false, disabledInChild: false, isEnabled: true),
TestCase(optedInInChild: true, disabledInChild: false, isEnabled: true),
TestCase(optedInInChild: false, disabledInChild: true, isEnabled: false),
TestCase(optedInInChild: true, disabledInChild: true, isEnabled: false)
]
XCTAssertEqual(testCases.unique.count, 2 * 2)
let ruleType = ImplicitReturnRule.self
XCTAssertTrue((ruleType as Any) is OptInRule.Type)
let ruleIdentifier = ruleType.description.identifier
let parentConfiguration = Configuration(rulesMode: .only([ruleIdentifier]))
for testCase in testCases {
let childConfiguration = Configuration(rulesMode: .default(
disabled: testCase.disabledInChild ? [ruleIdentifier] : [],
optIn: testCase.optedInInChild ? [ruleIdentifier] : []
))
let mergedConfiguration = parentConfiguration.merged(withChild: childConfiguration, rootDirectory: "")
let isEnabled = mergedConfiguration.contains(rule: ruleType)
XCTAssertEqual(isEnabled, testCase.isEnabled, testCase.message)
}
}

// MARK: - Remote Configs
func testValidRemoteChildConfig() {
FileManager.default.changeCurrentDirectoryPath(Mock.Dir.remoteConfigChild)
Expand Down
3 changes: 3 additions & 0 deletions Tests/SwiftLintFrameworkTests/Resources/ProjectMock/ChildConfig/Test2/expected.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ opt_in_rules:
- private_action

disabled_rules:
- empty_count
- force_cast
- private_outlet
- trailing_closure

line_length: 80

Expand Down
3 changes: 3 additions & 0 deletions Tests/SwiftLintFrameworkTests/Resources/ProjectMock/ParentConfig/Test2/expected.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ opt_in_rules:
- private_action

disabled_rules:
- empty_count
- force_cast
- private_outlet
- trailing_closure

line_length: 80

Expand Down