From 94701ac0ff706fc0b659c718aad0af5d3b13218b Mon Sep 17 00:00:00 2001 From: Kazumasa Shimomura Date: Wed, 12 Apr 2023 11:45:55 +0900 Subject: [PATCH 1/3] Fix prioritizeTodoOverMixedChinese --- .l10nlint.yml | 3 +- .../L10nLintFramework/Models/LintRunner.swift | 55 ++++++++++++++++++- .../Models/SourceKitten/Location.swift | 2 +- .../Models/SwiftLint/StyleViolation.swift | 4 +- .../L10nLintFramework/Rules/TodoRule.swift | 14 +---- .../LintRunnerTests.swift | 4 +- 6 files changed, 61 insertions(+), 21 deletions(-) diff --git a/.l10nlint.yml b/.l10nlint.yml index 41a6c5d..77b609a 100644 --- a/.l10nlint.yml +++ b/.l10nlint.yml @@ -1,4 +1,5 @@ -base_path: Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables4 +base_path: Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables7 +prioritize_todo_over_mixed_chinese: true disabled_rules: - empty_value diff --git a/Sources/L10nLintFramework/Models/LintRunner.swift b/Sources/L10nLintFramework/Models/LintRunner.swift index e709eac..9a00896 100644 --- a/Sources/L10nLintFramework/Models/LintRunner.swift +++ b/Sources/L10nLintFramework/Models/LintRunner.swift @@ -22,6 +22,7 @@ public final class LintRunner { } return rule } + let linters = projects.map { project in return Linter(baseProject: baseProject, project: project, rules: rules) } @@ -33,12 +34,62 @@ public final class LintRunner { guard configuration.prioritizeTodoOverMixedChinese else { return violations } let todoViolations = violations.filter { $0.ruleIdentifier == TodoRule.description.identifier } - return violations.filter { violation in - guard violation.ruleIdentifier == MixedChineseRule.description.identifier else { return true } + let filteredMixedChineseViolations = violations.filter { violation in + guard violation.ruleIdentifier == MixedChineseRule.description.identifier else { return false } return !todoViolations.contains(where: { todoViolation in violation.location.file == todoViolation.location.file && violation.location.line == todoViolation.location.line }) } + + let otherViolations = violations.filter { + $0.ruleIdentifier != MixedChineseRule.description.identifier && + $0.ruleIdentifier != TodoRule.description.identifier + } + + let todoConfiguration = configuration.ruleConfigurations.todo ?? .init() + if todoConfiguration.isSummaryEnabled && todoViolations.count > todoConfiguration.summaryViolationLimit { + var todoViolationsByFile = todoViolations.reduce(into: [String: [StyleViolation]]()) { partialResult, violation in + if partialResult[violation.location.file ?? ""] == nil { + partialResult[violation.location.file ?? ""] = [] + } + partialResult[violation.location.file ?? ""]?.append(violation) + } + + for violations in todoViolationsByFile { + let lines = violations.value.compactMap(\.location.line).map(String.init).joined(separator: ",") + if var violation = violations.value.first { + violation.location.line = 1 + violation.reason = "Lines has FIXME or TODO. line: \(lines)" + todoViolationsByFile[violations.key] = [violation] + } + } + + return todoViolationsByFile.flatMap(\.value) + filteredMixedChineseViolations + otherViolations + } + else { + return todoViolations + filteredMixedChineseViolations + otherViolations + } + } +} + +private extension [StyleViolation] { + func filterByPrioritizeTodoOverMixedChinese(for configuration: Configuration) -> [StyleViolation] { + guard configuration.prioritizeTodoOverMixedChinese else { return self } + let todoViolations = filter { $0.ruleIdentifier == TodoRule.description.identifier } + + let filteredMixedChineseViolations = filter { violation in + guard violation.ruleIdentifier == MixedChineseRule.description.identifier else { return false } + + return !todoViolations.contains(where: { todoViolation in + violation.location.file == todoViolation.location.file && violation.location.line == todoViolation.location.line + }) + } + + let otherViolations = filter { + $0.ruleIdentifier != MixedChineseRule.description.identifier && + $0.ruleIdentifier != TodoRule.description.identifier + } + return todoViolations + filteredMixedChineseViolations + otherViolations } } diff --git a/Sources/L10nLintFramework/Models/SourceKitten/Location.swift b/Sources/L10nLintFramework/Models/SourceKitten/Location.swift index 90d2944..3cfef65 100644 --- a/Sources/L10nLintFramework/Models/SourceKitten/Location.swift +++ b/Sources/L10nLintFramework/Models/SourceKitten/Location.swift @@ -5,7 +5,7 @@ public struct Location: CustomStringConvertible, Comparable, Codable { /// The file path on disk for this location. public let file: String? /// The line offset in the file for this location. 1-indexed. - public let line: Int? + public var line: Int? /// The character offset in the file for this location. 1-indexed. public let character: Int? diff --git a/Sources/L10nLintFramework/Models/SwiftLint/StyleViolation.swift b/Sources/L10nLintFramework/Models/SwiftLint/StyleViolation.swift index a9e9043..822325f 100644 --- a/Sources/L10nLintFramework/Models/SwiftLint/StyleViolation.swift +++ b/Sources/L10nLintFramework/Models/SwiftLint/StyleViolation.swift @@ -13,10 +13,10 @@ public struct StyleViolation: CustomStringConvertible, Equatable, Codable { public private(set) var severity: ViolationSeverity /// The location of this violation. - public private(set) var location: Location + public var location: Location /// The justification for this violation. - public let reason: String + public var reason: String /// A printable description for this violation. public var description: String { diff --git a/Sources/L10nLintFramework/Rules/TodoRule.swift b/Sources/L10nLintFramework/Rules/TodoRule.swift index e7f7a23..38aede4 100644 --- a/Sources/L10nLintFramework/Rules/TodoRule.swift +++ b/Sources/L10nLintFramework/Rules/TodoRule.swift @@ -24,19 +24,7 @@ public struct TodoRule: ConfigurationProviderRule { ) } - if configuration.isSummaryEnabled && dataSets.count > configuration.summaryViolationLimit { - let lines = dataSets.compactMap(\.violation.location.line).map(String.init).joined(separator: ",") - - return [StyleViolation( - ruleDescription: Self.description, - severity: .warning, - location: Location(file: project.stringsFile.path, line: 1), - reason: "Lines has FIXME or TODO. line: \(lines)" - )] - } - else { - return dataSets.map(\.violation) - } + return dataSets.map(\.violation) } } diff --git a/Tests/L10nLintFrameworkTests/LintRunnerTests.swift b/Tests/L10nLintFrameworkTests/LintRunnerTests.swift index 3ed590a..23687df 100644 --- a/Tests/L10nLintFrameworkTests/LintRunnerTests.swift +++ b/Tests/L10nLintFrameworkTests/LintRunnerTests.swift @@ -22,8 +22,8 @@ final class LintRunnerTests: XCTestCase { )) XCTAssertEqual(violations.map(\.ruleWithLocationPoint), [ RuleWithLocationPoint(ruleIdentifier: "todo", file: "zh-Hans.lproj", line: 2, character: 1), - RuleWithLocationPoint(ruleIdentifier: "mixed_chinese", file: "zh-Hans.lproj", line: 3, character: 12), - RuleWithLocationPoint(ruleIdentifier: "todo", file: "zh-Hant.lproj", line: 2, character: 1) + RuleWithLocationPoint(ruleIdentifier: "todo", file: "zh-Hant.lproj", line: 2, character: 1), + RuleWithLocationPoint(ruleIdentifier: "mixed_chinese", file: "zh-Hans.lproj", line: 3, character: 12) ]) } } From 5b539b18d54adb25f6381b0b917653e6e4bf8eba Mon Sep 17 00:00:00 2001 From: Kazumasa Shimomura Date: Wed, 12 Apr 2023 11:50:05 +0900 Subject: [PATCH 2/3] Refactor --- .../L10nLintFramework/Models/LintRunner.swift | 67 ++++++++----------- 1 file changed, 28 insertions(+), 39 deletions(-) diff --git a/Sources/L10nLintFramework/Models/LintRunner.swift b/Sources/L10nLintFramework/Models/LintRunner.swift index 9a00896..b339385 100644 --- a/Sources/L10nLintFramework/Models/LintRunner.swift +++ b/Sources/L10nLintFramework/Models/LintRunner.swift @@ -31,45 +31,9 @@ public final class LintRunner { try $0.lint() } - guard configuration.prioritizeTodoOverMixedChinese else { return violations } - let todoViolations = violations.filter { $0.ruleIdentifier == TodoRule.description.identifier } - - let filteredMixedChineseViolations = violations.filter { violation in - guard violation.ruleIdentifier == MixedChineseRule.description.identifier else { return false } - - return !todoViolations.contains(where: { todoViolation in - violation.location.file == todoViolation.location.file && violation.location.line == todoViolation.location.line - }) - } - - let otherViolations = violations.filter { - $0.ruleIdentifier != MixedChineseRule.description.identifier && - $0.ruleIdentifier != TodoRule.description.identifier - } - - let todoConfiguration = configuration.ruleConfigurations.todo ?? .init() - if todoConfiguration.isSummaryEnabled && todoViolations.count > todoConfiguration.summaryViolationLimit { - var todoViolationsByFile = todoViolations.reduce(into: [String: [StyleViolation]]()) { partialResult, violation in - if partialResult[violation.location.file ?? ""] == nil { - partialResult[violation.location.file ?? ""] = [] - } - partialResult[violation.location.file ?? ""]?.append(violation) - } - - for violations in todoViolationsByFile { - let lines = violations.value.compactMap(\.location.line).map(String.init).joined(separator: ",") - if var violation = violations.value.first { - violation.location.line = 1 - violation.reason = "Lines has FIXME or TODO. line: \(lines)" - todoViolationsByFile[violations.key] = [violation] - } - } - - return todoViolationsByFile.flatMap(\.value) + filteredMixedChineseViolations + otherViolations - } - else { - return todoViolations + filteredMixedChineseViolations + otherViolations - } + return violations + .filterByPrioritizeTodoOverMixedChinese(for: configuration) + .modifyByTodoSummary(for: configuration) } } @@ -92,4 +56,29 @@ private extension [StyleViolation] { } return todoViolations + filteredMixedChineseViolations + otherViolations } + + func modifyByTodoSummary(for configuration: Configuration) -> [StyleViolation] { + let todoConfiguration = configuration.ruleConfigurations.todo ?? .init() + let todoViolations = filter { $0.ruleIdentifier == TodoRule.description.identifier } + let otherViolations = filter { $0.ruleIdentifier != TodoRule.description.identifier } + + guard todoConfiguration.isSummaryEnabled && todoViolations.count > todoConfiguration.summaryViolationLimit else { return self } + var todoViolationsByFile = todoViolations.reduce(into: [String: [StyleViolation]]()) { partialResult, violation in + if partialResult[violation.location.file ?? ""] == nil { + partialResult[violation.location.file ?? ""] = [] + } + partialResult[violation.location.file ?? ""]?.append(violation) + } + + for violations in todoViolationsByFile { + let lines = violations.value.compactMap(\.location.line).map(String.init).joined(separator: ",") + if var violation = violations.value.first { + violation.location.line = 1 + violation.reason = "Lines has FIXME or TODO. line: \(lines)" + todoViolationsByFile[violations.key] = [violation] + } + } + + return todoViolationsByFile.flatMap(\.value) + otherViolations + } } From c4037711484971453314636490a448b77f50ad3c Mon Sep 17 00:00:00 2001 From: Kazumasa Shimomura Date: Wed, 12 Apr 2023 12:03:47 +0900 Subject: [PATCH 3/3] Add tests --- .../LintRunnerTests.swift | 50 ++++++++++++++++++- .../Base.lproj/Localizable.strings | 6 +++ .../zh-Hans.lproj/Localizable.strings | 6 +++ 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/Base.lproj/Localizable.strings create mode 100644 Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/zh-Hans.lproj/Localizable.strings diff --git a/Tests/L10nLintFrameworkTests/LintRunnerTests.swift b/Tests/L10nLintFrameworkTests/LintRunnerTests.swift index 23687df..a3fa24d 100644 --- a/Tests/L10nLintFrameworkTests/LintRunnerTests.swift +++ b/Tests/L10nLintFrameworkTests/LintRunnerTests.swift @@ -2,7 +2,7 @@ import XCTest @testable import L10nLintFramework final class LintRunnerTests: XCTestCase { - func testPreferFixMeThanMixedChineseFalse() throws { + func testPrioritizeTodoOverMixedChineseFalse() throws { let violations = try LintRunner.run(configuration: Configuration( basePath: TestHelper.fixtureURL(fixtureName: "Localizables7").path, prioritizeTodoOverMixedChinese: false @@ -15,7 +15,7 @@ final class LintRunnerTests: XCTestCase { ]) } - func testPreferFixMeThanMixedChineseTrue() throws { + func testPrioritizeTodoOverMixedChineseTrue() throws { let violations = try LintRunner.run(configuration: Configuration( basePath: TestHelper.fixtureURL(fixtureName: "Localizables7").path, prioritizeTodoOverMixedChinese: true @@ -26,6 +26,52 @@ final class LintRunnerTests: XCTestCase { RuleWithLocationPoint(ruleIdentifier: "mixed_chinese", file: "zh-Hans.lproj", line: 3, character: 12) ]) } + + func testTodoSummaryTrue() throws { + let violations = try LintRunner.run(configuration: Configuration( + basePath: TestHelper.fixtureURL(fixtureName: "Localizables8").path, + prioritizeTodoOverMixedChinese: false, + ruleConfigurations: .init(todo: .init( + isSummaryEnabled: true, + summaryViolationLimit: 3 + )) + )) + XCTAssertEqual(violations.map(\.ruleWithLocationPoint), [ + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 1, character: 1) + ]) + } + + func testTodoSummaryTrueLimit5() throws { + let violations = try LintRunner.run(configuration: Configuration( + basePath: TestHelper.fixtureURL(fixtureName: "Localizables8").path, + prioritizeTodoOverMixedChinese: false, + ruleConfigurations: .init(todo: .init( + isSummaryEnabled: true, + summaryViolationLimit: 4 + )) + )) + XCTAssertEqual(violations.map(\.ruleWithLocationPoint), [ + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 1, character: 1) + ]) + } + + func testTodoSummaryFalse() throws { + let violations = try LintRunner.run(configuration: Configuration( + basePath: TestHelper.fixtureURL(fixtureName: "Localizables8").path, + prioritizeTodoOverMixedChinese: false, + ruleConfigurations: .init(todo: .init( + isSummaryEnabled: false, + summaryViolationLimit: 3 + )) + )) + XCTAssertEqual(violations.map(\.ruleWithLocationPoint), [ + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 2, character: 1), + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 3, character: 1), + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 4, character: 1), + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 5, character: 1), + RuleWithLocationPoint(ruleIdentifier: "todo", file: "Base.lproj", line: 6, character: 1) + ]) + } } private struct RuleWithLocationPoint: Equatable, CustomStringConvertible { diff --git a/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/Base.lproj/Localizable.strings b/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/Base.lproj/Localizable.strings new file mode 100644 index 0000000..2b06236 --- /dev/null +++ b/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/Base.lproj/Localizable.strings @@ -0,0 +1,6 @@ +// MARK: TodoSummary +"Key1" = "FIXME: A"; +"Key2" = "FIXME: B"; +"Key3" = "FIXME: C"; +"Key4" = "FIXME: D"; +"Key5" = "FIXME: E"; diff --git a/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/zh-Hans.lproj/Localizable.strings b/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/zh-Hans.lproj/Localizable.strings new file mode 100644 index 0000000..a226993 --- /dev/null +++ b/Tests/L10nLintFrameworkTests/Resources/Fixtures/Localizables8/zh-Hans.lproj/Localizable.strings @@ -0,0 +1,6 @@ +// MARK: TodoSummary +"Key1" = "A"; +"Key2" = "B"; +"Key3" = "C"; +"Key4" = "D"; +"Key5" = "E";