From 6a38b156579743baf410705665046ff7b51156e5 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Fri, 22 Jan 2021 13:31:05 -0500 Subject: [PATCH] Revert "Merge pull request #3473 from realm/feature/3472-fix-custom-rules-merging" (#3503) This reverts commit 537e53f6b395b7aad3aa9b3ee3588f820359d330, reversing changes made to ba49f7d3097d51b7f55b8df8b8a9f4c874fc5d79. --- CHANGELOG.md | 15 --- ...rapper.swift => Configuration+Rules.swift} | 108 +++++++----------- .../ConfigurationTests+Mock.swift | 10 -- .../ConfigurationTests+MultipleConfigs.swift | 50 +------- .../Level1/Level2/custom_rules_only.yml | 4 - .../Level1/Level2/custom_rules_reconfig.yml | 5 - .../ProjectMock/custom_rules_only.yml | 7 -- 7 files changed, 45 insertions(+), 154 deletions(-) rename Source/SwiftLintFramework/Extensions/{Configuration+RulesWrapper.swift => Configuration+Rules.swift} (72%) delete mode 100644 Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml delete mode 100644 Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml delete mode 100644 Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 9866cd1605..ea247e7d2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,21 +27,6 @@ #### Bug Fixes -* Fix `custom_rules` merging when the parent configuration is based on - `only_rules`. - [Frederick Pietschmann](https://github.com/fredpi) - [#3468](https://github.com/realm/SwiftLint/issues/3468) - -* Fix misleading warnings about rules defined in the `custom_rules` not - being available (when using multiple configurations). - [Frederick Pietschmann](https://github.com/fredpi) - [#3472](https://github.com/realm/SwiftLint/issues/3472) - -* Fix bug that prevented the reconfiguration of a custom rule in a child - config. - [Frederick Pietschmann](https://github.com/fredpi) - [#3477](https://github.com/realm/SwiftLint/issues/3477) - * Fix typos in configuration options for `file_name` rule. [advantis](https://github.com/advantis) diff --git a/Source/SwiftLintFramework/Extensions/Configuration+RulesWrapper.swift b/Source/SwiftLintFramework/Extensions/Configuration+Rules.swift similarity index 72% rename from Source/SwiftLintFramework/Extensions/Configuration+RulesWrapper.swift rename to Source/SwiftLintFramework/Extensions/Configuration+Rules.swift index 229046ab56..cada0326e3 100644 --- a/Source/SwiftLintFramework/Extensions/Configuration+RulesWrapper.swift +++ b/Source/SwiftLintFramework/Extensions/Configuration+Rules.swift @@ -32,22 +32,18 @@ internal extension Configuration { if let cachedResultingRules = cachedResultingRules { return cachedResultingRules } // Calculate value - let customRulesFilter: (RegexConfiguration) -> (Bool) var resultingRules = [Rule]() switch mode { case .allEnabled: - customRulesFilter = { _ in true } resultingRules = allRulesWrapped.map { $0.rule } case var .only(onlyRulesRuleIdentifiers): - customRulesFilter = { onlyRulesRuleIdentifiers.contains($0.identifier) } onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers) resultingRules = allRulesWrapped.filter { tuple in onlyRulesRuleIdentifiers.contains(type(of: tuple.rule).description.identifier) }.map { $0.rule } case var .default(disabledRuleIdentifiers, optInRuleIdentifiers): - customRulesFilter = { !disabledRuleIdentifiers.contains($0.identifier) } disabledRuleIdentifiers = validate(ruleIds: disabledRuleIdentifiers, valid: validRuleIdentifiers) optInRuleIdentifiers = validate(ruleIds: optInRuleIdentifiers, valid: validRuleIdentifiers) resultingRules = allRulesWrapped.filter { tuple in @@ -57,13 +53,6 @@ internal extension Configuration { }.map { $0.rule } } - // Filter custom rules - if var customRulesRule = (resultingRules.first { $0 is CustomRules }) as? CustomRules { - customRulesRule.configuration.customRuleConfigurations = - customRulesRule.configuration.customRuleConfigurations.filter(customRulesFilter) - resultingRules = resultingRules.filter { !($0 is CustomRules) } + [customRulesRule] - } - // Sort by name resultingRules = resultingRules.sorted { type(of: $0).description.identifier < type(of: $1).description.identifier @@ -142,22 +131,10 @@ internal extension Configuration { newAllRulesWrapped: newAllRulesWrapped, child: child, childDisabled: childDisabled, - childOptIn: childOptIn, - validRuleIdentifiers: validRuleIdentifiers + childOptIn: childOptIn ) case var .only(childOnlyRules): - // If the custom_rules rule is enabled, add all rules defined by the child's custom_rules rule - if (childOnlyRules.contains { $0 == CustomRules.description.identifier }), - let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule - as? CustomRules { - childOnlyRules = childOnlyRules.union( - Set( - childCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier } - ) - ) - } - childOnlyRules = child.validate(ruleIds: childOnlyRules, valid: validRuleIdentifiers) // Always use the child only rules @@ -168,17 +145,21 @@ internal extension Configuration { newMode = .allEnabled } - // Assemble & return merged rules + // Assemble & return merged Rules return RulesWrapper( mode: newMode, - allRulesWrapped: mergedCustomRules(newAllRulesWrapped: newAllRulesWrapped, with: child), + allRulesWrapped: merged( + customRules: newAllRulesWrapped, + mode: newMode, + with: child + ), aliasResolver: { child.aliasResolver(self.aliasResolver($0)) } ) } - private func mergedAllRulesWrapped(with child: RulesWrapper) -> [ConfigurationRuleWrapper] { + private func mergedAllRulesWrapped(with sub: RulesWrapper) -> [ConfigurationRuleWrapper] { let mainConfigSet = Set(allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init)) - let childConfigSet = Set(child.allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init)) + let childConfigSet = Set(sub.allRulesWrapped.map(HashableConfigurationRuleWrapperWrapper.init)) let childConfigRulesWithConfig = childConfigSet.filter { $0.configurationRuleWrapper.initializedWithNonEmptyConfiguration } @@ -190,44 +171,54 @@ internal extension Configuration { .map { $0.configurationRuleWrapper } } - private func mergedCustomRules( - newAllRulesWrapped: [ConfigurationRuleWrapper], with child: RulesWrapper + private func merged( + customRules rules: [ConfigurationRuleWrapper], mode: RulesMode, with child: RulesWrapper ) -> [ConfigurationRuleWrapper] { guard - let parentCustomRulesRule = (allRulesWrapped.first { $0.rule is CustomRules })?.rule - as? CustomRules, - let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule - as? CustomRules + let customRulesRule = (allRulesWrapped.first { + $0.rule is CustomRules + })?.rule as? CustomRules, + let childCustomRulesRule = (child.allRulesWrapped.first { + $0.rule is CustomRules + })?.rule as? CustomRules else { // Merging is only needed if both parent & child have a custom rules rule - return newAllRulesWrapped + return rules + } + + let customRulesFilter: (RegexConfiguration) -> (Bool) + switch mode { + case .allEnabled: + customRulesFilter = { _ in true } + + case let .only(onlyRules): + customRulesFilter = { onlyRules.contains($0.identifier) } + + case let .default(disabledRules, _): + customRulesFilter = { !disabledRules.contains($0.identifier) } } - // Create new custom rules rule, prioritizing child custom rules var configuration = CustomRulesConfiguration() - configuration.customRuleConfigurations = childCustomRulesRule.configuration.customRuleConfigurations - + parentCustomRulesRule.configuration.customRuleConfigurations.filter { parentConfig in - !childCustomRulesRule.configuration.customRuleConfigurations.contains { childConfig in - childConfig.identifier == parentConfig.identifier - } - } - var newCustomRulesRule = CustomRules() - newCustomRulesRule.configuration = configuration + configuration.customRuleConfigurations = Set(customRulesRule.configuration.customRuleConfigurations) + .union(Set(childCustomRulesRule.configuration.customRuleConfigurations)) + .filter(customRulesFilter) + + var customRules = CustomRules() + customRules.configuration = configuration - return newAllRulesWrapped.filter { !($0.rule is CustomRules) } + [(newCustomRulesRule, true)] + return rules.filter { !($0.rule is CustomRules) } + [(customRules, true)] } private func mergeDefaultMode( newAllRulesWrapped: [ConfigurationRuleWrapper], child: RulesWrapper, childDisabled: Set, - childOptIn: Set, - validRuleIdentifiers: Set + childOptIn: Set ) -> RulesMode { let childDisabled = child.validate(ruleIds: childDisabled, valid: validRuleIdentifiers) let childOptIn = child.validate(ruleIds: childOptIn, valid: validRuleIdentifiers) - switch mode { // Switch parent's mode. Child is in default mode. + switch mode { case var .default(disabled, optIn): disabled = validate(ruleIds: disabled, valid: validRuleIdentifiers) optIn = child.validate(ruleIds: optIn, valid: validRuleIdentifiers) @@ -247,27 +238,6 @@ internal extension Configuration { ) case var .only(onlyRules): - // Add identifiers of custom rules iff the custom_rules rule is enabled - if (onlyRules.contains { $0 == CustomRules.description.identifier }) { - if let childCustomRulesRule = (child.allRulesWrapped.first { $0.rule is CustomRules })?.rule - as? CustomRules { - onlyRules = onlyRules.union( - Set( - childCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier } - ) - ) - } - - if let parentCustomRulesRule = (self.allRulesWrapped.first { $0.rule is CustomRules })?.rule - as? CustomRules { - onlyRules = onlyRules.union( - Set( - parentCustomRulesRule.configuration.customRuleConfigurations.map { $0.identifier } - ) - ) - } - } - onlyRules = validate(ruleIds: onlyRules, valid: validRuleIdentifiers) // Allow parent only rules that weren't disabled via the child config diff --git a/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift b/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift index aa459dcacd..1a62250d73 100644 --- a/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift +++ b/Tests/SwiftLintFrameworkTests/ConfigurationTests+Mock.swift @@ -38,16 +38,11 @@ internal extension ConfigurationTests { static var _0: String { Dir.level0.stringByAppendingPathComponent(Configuration.defaultFileName) } static var _0CustomPath: String { Dir.level0.stringByAppendingPathComponent("custom.yml") } static var _0CustomRules: String { Dir.level0.stringByAppendingPathComponent("custom_rules.yml") } - static var _0CustomRulesOnly: String { Dir.level0.stringByAppendingPathComponent("custom_rules_only.yml") } static var _2: String { Dir.level2.stringByAppendingPathComponent(Configuration.defaultFileName) } static var _2CustomRules: String { Dir.level2.stringByAppendingPathComponent("custom_rules.yml") } - static var _2CustomRulesOnly: String { Dir.level2.stringByAppendingPathComponent("custom_rules_only.yml") } static var _2CustomRulesDisabled: String { Dir.level2.stringByAppendingPathComponent("custom_rules_disabled.yml") } - static var _2CustomRulesReconfig: String { - Dir.level2.stringByAppendingPathComponent("custom_rules_reconfig.yml") - } static var _3: String { Dir.level3.stringByAppendingPathComponent(Configuration.defaultFileName) } static var nested: String { Dir.nested.stringByAppendingPathComponent(Configuration.defaultFileName) } } @@ -66,16 +61,11 @@ internal extension ConfigurationTests { static var _0: Configuration { Configuration(configurationFiles: []) } static var _0CustomPath: Configuration { Configuration(configurationFiles: [Yml._0CustomPath]) } static var _0CustomRules: Configuration { Configuration(configurationFiles: [Yml._0CustomRules]) } - static var _0CustomRulesOnly: Configuration { Configuration(configurationFiles: [Yml._0CustomRulesOnly]) } static var _2: Configuration { Configuration(configurationFiles: [Yml._2]) } static var _2CustomRules: Configuration { Configuration(configurationFiles: [Yml._2CustomRules]) } - static var _2CustomRulesOnly: Configuration { Configuration(configurationFiles: [Yml._2CustomRulesOnly]) } static var _2CustomRulesDisabled: Configuration { Configuration(configurationFiles: [Yml._2CustomRulesDisabled]) } - static var _2CustomRulesReconfig: Configuration { - Configuration(configurationFiles: [Yml._2CustomRulesReconfig]) - } static var _3: Configuration { Configuration(configurationFiles: [Yml._3]) } static var nested: Configuration { Configuration(configurationFiles: [Yml.nested]) } } diff --git a/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift b/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift index 458b0adfe3..c94054834a 100644 --- a/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift +++ b/Tests/SwiftLintFrameworkTests/ConfigurationTests+MultipleConfigs.swift @@ -76,13 +76,13 @@ extension ConfigurationTests { ) guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules else { - return XCTFail("Custom rules are expected to be present") + return XCTFail("Custom rule are expected to be present") } XCTAssertTrue( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" } + mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abc" }) ) XCTAssertTrue( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" } + mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abcd" }) ) } @@ -93,54 +93,16 @@ extension ConfigurationTests { ) guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules else { - return XCTFail("Custom rules are expected to be present") + return XCTFail("Custom rule are expected to be present") } XCTAssertFalse( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" } + mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abc" }) ) XCTAssertTrue( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" } + mergedCustomRules.configuration.customRuleConfigurations.contains(where: { $0.identifier == "no_abcd" }) ) } - func testCustomRulesMergingWithOnlyRules() { - let mergedConfiguration = Mock.Config._0CustomRulesOnly.merged( - withChild: Mock.Config._2CustomRulesOnly, - rootDirectory: "" - ) - guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules - else { - return XCTFail("Custom rules are expected to be present") - } - XCTAssertTrue( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abc" } - ) - XCTAssertTrue( - mergedCustomRules.configuration.customRuleConfigurations.contains { $0.identifier == "no_abcd" } - ) - } - - func testCustomRulesReconfiguration() { - // Custom Rule severity gets reconfigured to "error" - let mergedConfiguration = Mock.Config._0CustomRulesOnly.merged( - withChild: Mock.Config._2CustomRulesReconfig, - rootDirectory: "" - ) - guard let mergedCustomRules = mergedConfiguration.rules.first(where: { $0 is CustomRules }) as? CustomRules - else { - return XCTFail("Custom rules are expected to be present") - } - XCTAssertEqual( - mergedCustomRules.configuration.customRuleConfigurations.filter { $0.identifier == "no_abc" }.count, 1 - ) - guard let customRule = (mergedCustomRules.configuration.customRuleConfigurations.first { - $0.identifier == "no_abc" - }) else { - return XCTFail("Custom rule is expected to be present") - } - XCTAssertEqual(customRule.severityConfiguration.severity, .error) - } - // MARK: - Nested Configurations func testLevel0() { XCTAssertEqual(Mock.Config._0.configuration(for: SwiftLintFile(path: Mock.Swift._0)!), diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml deleted file mode 100644 index 3f92454396..0000000000 --- a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_only.yml +++ /dev/null @@ -1,4 +0,0 @@ -custom_rules: - no_abcd: - name: "Don't use abcd" - regex: 'abcd' diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml deleted file mode 100644 index 947b02b195..0000000000 --- a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/custom_rules_reconfig.yml +++ /dev/null @@ -1,5 +0,0 @@ -custom_rules: - no_abc: - name: "Don't use abc" - regex: 'abc' - severity: 'error' diff --git a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml b/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml deleted file mode 100644 index 5ed1eea557..0000000000 --- a/Tests/SwiftLintFrameworkTests/Resources/ProjectMock/custom_rules_only.yml +++ /dev/null @@ -1,7 +0,0 @@ -only_rules: - - custom_rules - -custom_rules: - no_abc: - name: "Don't use abc" - regex: 'abc'