Skip to content

Commit

Permalink
Make postprocessors non-throwing (#5539)
Browse files Browse the repository at this point in the history 
Failing immediately when a property is invalid is too strict. It feels
sufficient to allow to report an issue but otherwise continue with a default
value instead of stopping execution completely.
  • Loading branch information
@SimplyDanny
SimplyDanny authored Apr 23, 2024
1 parent 3276266 commit 1b7fbc4
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 28 deletions.
9 changes: 8 additions & 1 deletion Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/IndentationWidthConfiguration.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,18 @@ import SwiftLintCore
struct IndentationWidthConfiguration: SeverityBasedRuleConfiguration {
typealias Parent = IndentationWidthRule

private static let defaultIndentationWidth = 4

@ConfigurationElement(key: "severity")
private(set) var severityConfiguration = SeverityConfiguration<Parent>.warning
@ConfigurationElement(
key: "indentation_width",
postprocessor: { if $0 < 1 { throw Issue.invalidConfiguration(ruleID: Parent.identifier) } }
postprocessor: {
if $0 < 1 {
Issue.invalidConfiguration(ruleID: Parent.identifier).print()
$0 = Self.defaultIndentationWidth
}
}
)
private(set) var indentationWidth = 4
@ConfigurationElement(key: "include_comments")
Expand Down
23 changes: 12 additions & 11 deletions Source/SwiftLintCore/Models/RuleConfigurationDescription.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,13 @@ public protocol InlinableOptionType: AcceptableByConfigurationElement {}
@propertyWrapper
public struct ConfigurationElement<T: AcceptableByConfigurationElement & Equatable>: Equatable {
/// Wrapped option value.
public var wrappedValue: T
public var wrappedValue: T {
didSet {
if wrappedValue != oldValue {
postprocessor(&wrappedValue)
}
}
}

/// The wrapper itself providing access to all its data. This field can only be accessed by the
/// element's name prefixed with a `$`.
Expand All @@ -431,7 +437,7 @@ public struct ConfigurationElement<T: AcceptableByConfigurationElement & Equatab
/// Whether this configuration element will be inlined into its description.
public let inline: Bool

private let postprocessor: (inout T) throws -> Void
private let postprocessor: (inout T) -> Void

/// Default constructor.
///
Expand All @@ -441,11 +447,11 @@ public struct ConfigurationElement<T: AcceptableByConfigurationElement & Equatab
/// - postprocessor: Function to be applied to the wrapped value after parsing to validate and modify it.
public init(wrappedValue value: T,
key: String,
postprocessor: @escaping (inout T) throws -> Void = { _ in }) {
postprocessor: @escaping (inout T) -> Void = { _ in }) {
self.init(wrappedValue: value, key: key, inline: false, postprocessor: postprocessor)

// Validate and modify the set value immediately. An exception means invalid defaults.
try! performAfterParseOperations() // swiftlint:disable:this force_try
// Modify the set value immediately.
postprocessor(&wrappedValue)
}

/// Constructor for optional values.
Expand Down Expand Up @@ -482,18 +488,13 @@ public struct ConfigurationElement<T: AcceptableByConfigurationElement & Equatab
private init(wrappedValue: T,
key: String,
inline: Bool,
postprocessor: @escaping (inout T) throws -> Void = { _ in }) {
postprocessor: @escaping (inout T) -> Void = { _ in }) {
self.wrappedValue = wrappedValue
self.key = key
self.inline = inline
self.postprocessor = postprocessor
}

/// Run operations to validate and modify the parsed value.
public mutating func performAfterParseOperations() throws {
try postprocessor(&wrappedValue)
}

public static func == (lhs: ConfigurationElement, rhs: ConfigurationElement) -> Bool {
lhs.wrappedValue == rhs.wrappedValue && lhs.key == rhs.key
}
Expand Down
4 changes: 0 additions & 4 deletions Source/SwiftLintCoreMacros/RuleConfigurationMacros.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ enum AutoApply: MemberMacro {
"""
do {
try \(raw: option).apply(configuration, ruleID: Parent.identifier)
try $\(raw: option).performAfterParseOperations()
} catch let issue as Issue where issue == Issue.nothingApplied(ruleID: Parent.identifier) {
// Acceptable. Continue.
}
Expand All @@ -77,9 +76,6 @@ enum AutoApply: MemberMacro {
"""
try \(raw: option).apply(configuration[$\(raw: option).key], ruleID: Parent.identifier)
"""
"""
try $\(raw: option).performAfterParseOperations()
"""
}
"""
if !supportedKeys.isSuperset(of: configuration.keys) {
Expand Down
8 changes: 0 additions & 8 deletions Tests/MacroTests/AutoApplyTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,7 @@ final class AutoApplyTests: XCTestCase {
throw Issue.invalidConfiguration(ruleID: Parent.identifier)
}
try eA.apply(configuration[$eA.key], ruleID: Parent.identifier)
try $eA.performAfterParseOperations()
try eB.apply(configuration[$eB.key], ruleID: Parent.identifier)
try $eB.performAfterParseOperations()
if !supportedKeys.isSuperset(of: configuration.keys) {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
throw Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys)
Expand Down Expand Up @@ -127,17 +125,14 @@ final class AutoApplyTests: XCTestCase {
}
do {
try eB.apply(configuration, ruleID: Parent.identifier)
try $eB.performAfterParseOperations()
} catch let issue as Issue where issue == Issue.nothingApplied(ruleID: Parent.identifier) {
// Acceptable. Continue.
}
guard let configuration = configuration as? [String: Any] else {
return
}
try eA.apply(configuration[$eA.key], ruleID: Parent.identifier)
try $eA.performAfterParseOperations()
try eC.apply(configuration[$eC.key], ruleID: Parent.identifier)
try $eC.performAfterParseOperations()
if !supportedKeys.isSuperset(of: configuration.keys) {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
throw Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys)
Expand Down Expand Up @@ -210,17 +205,14 @@ final class AutoApplyTests: XCTestCase {
}
do {
try severityConfiguration.apply(configuration, ruleID: Parent.identifier)
try $severityConfiguration.performAfterParseOperations()
} catch let issue as Issue where issue == Issue.nothingApplied(ruleID: Parent.identifier) {
// Acceptable. Continue.
}
guard let configuration = configuration as? [String: Any] else {
return
}
try severityConfiguration.apply(configuration[$severityConfiguration.key], ruleID: Parent.identifier)
try $severityConfiguration.performAfterParseOperations()
try foo.apply(configuration[$foo.key], ruleID: Parent.identifier)
try $foo.performAfterParseOperations()
if !supportedKeys.isSuperset(of: configuration.keys) {
let unknownKeys = Set(configuration.keys).subtracting(supportedKeys)
throw Issue.invalidConfigurationKeys(ruleID: Parent.identifier, keys: unknownKeys)
Expand Down
10 changes: 6 additions & 4 deletions Tests/SwiftLintFrameworkTests/IndentationWidthRuleTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import SwiftLintTestHelpers
import XCTest

class IndentationWidthRuleTests: SwiftLintTestCase {
func testInvalidIndentation() {
func testInvalidIndentation() throws {
var testee = IndentationWidthConfiguration()
let defaultValue = testee.indentationWidth
for indentation in [0, -1, -5] {
checkError(Issue.invalidConfiguration(ruleID: IndentationWidthRule.description.identifier)) {
try testee.apply(configuration: ["indentation_width": indentation])
}
try testee.apply(configuration: ["indentation_width": indentation])

// Value remains the default.
XCTAssertEqual(testee.indentationWidth, defaultValue)
}
}

Expand Down

0 comments on commit 1b7fbc4

Please sign in to comment.