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.

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

Add validation hook to configuration parsing #5824

Merged
merged 1 commit into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions Source/SwiftLintCore/Models/Issue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Foundation
/// All possible SwiftLint issues which are printed as warnings by default.
public enum Issue: LocalizedError, Equatable {
/// The configuration didn't match internal expectations.
case invalidConfiguration(ruleID: String)
case invalidConfiguration(ruleID: String, message: String? = nil)
Copy link
Collaborator

@mildm8nnered mildm8nnered Oct 17, 2024

Choose a reason for hiding this comment

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

I wonder if we could help the reader more with distinguishing between invalid rule configurations, and invalid swiftlint configurations (if you consider those to be different things) - invalidRuleConfiguration although I guess that is implied by ruleID). There are some other issues where we could also make that distinction if we wanted to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, I'm always a fan of tools providing clear feedback. This one can only be used inside of rules due to the ruleID parameter which is not otherwise available.


/// Issued when an option is deprecated. Suggests an alternative optionally.
case deprecatedConfigurationOption(ruleID: String, key: String, alternative: String? = nil)
Expand All @@ -20,6 +20,10 @@ public enum Issue: LocalizedError, Equatable {
/// Some configuration keys are invalid.
case invalidConfigurationKeys(ruleID: String, keys: Set<String>)

/// The configuration is inconsistent, that is options are mutually exclusive or one drives other values
/// irrelevant.
case inconsistentConfiguration(ruleID: String, message: String)

/// Used rule IDs are invalid.
case invalidRuleIDs(Set<String>)

Expand Down Expand Up @@ -138,8 +142,9 @@ public enum Issue: LocalizedError, Equatable {

private var message: String {
switch self {
case let .invalidConfiguration(id):
return "Invalid configuration for '\(id)' rule. Falling back to default."
case let .invalidConfiguration(id, message):
let message = if let message { ": \(message)" } else { "." }
return "Invalid configuration for '\(id)' rule\(message) Falling back to default."
case let .deprecatedConfigurationOption(id, key, alternative):
let baseMessage = "Configuration option '\(key)' in '\(id)' rule is deprecated."
if let alternative {
Expand All @@ -154,6 +159,8 @@ public enum Issue: LocalizedError, Equatable {
return "'\(old)' has been renamed to '\(new)' and will be completely removed in a future release."
case let .invalidConfigurationKeys(id, keys):
return "Configuration for '\(id)' rule contains the invalid key(s) \(keys.formatted)."
case let .inconsistentConfiguration(id, message):
return "Inconsistent configuration for '\(id)' rule: \(message)"
case let .invalidRuleIDs(ruleIDs):
return "The key(s) \(ruleIDs.formatted) used as rule identifier(s) is/are invalid."
case let .ruleNotPresentInOnlyRules(id):
Expand Down
8 changes: 8 additions & 0 deletions Source/SwiftLintCore/Protocols/RuleConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ public protocol RuleConfiguration: Equatable {
///
/// - throws: Throws if the configuration is not in the expected format.
mutating func apply(configuration: Any) throws

/// Run a sanity check on the configuration, perform optional postprocessing steps and/or warn about potential
/// issues.
mutating func validate() throws
}

/// A configuration for a rule that allows to configure at least the severity.
Expand All @@ -30,6 +34,10 @@ public extension SeverityBasedRuleConfiguration {

public extension RuleConfiguration {
var parameterDescription: RuleConfigurationDescription? { nil }

func validate() throws {
// Do nothing by default.
}
}

public extension RuleConfiguration {
Expand Down
3 changes: 3 additions & 0 deletions Source/SwiftLintCoreMacros/RuleConfigurationMacros.swift
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ enum AutoConfigParser: MemberMacro {
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
"""
"""
try validate()
"""
}),
]
}
Expand Down
5 changes: 5 additions & 0 deletions Tests/MacroTests/AutoConfigParserTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ final class AutoConfigParserTests: XCTestCase {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
try validate()
}
}
""",
Expand Down Expand Up @@ -90,6 +91,7 @@ final class AutoConfigParserTests: XCTestCase {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
try validate()
}
}
""",
Expand Down Expand Up @@ -145,6 +147,7 @@ final class AutoConfigParserTests: XCTestCase {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
try validate()
}
}
""",
Expand All @@ -171,6 +174,7 @@ final class AutoConfigParserTests: XCTestCase {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
try validate()
}
}
""",
Expand Down Expand Up @@ -228,6 +232,7 @@ final class AutoConfigParserTests: XCTestCase {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys).print()
}
try validate()
}
}
""",
Expand Down