Skip to content

Refactor Exclude by behavior into a Strategy pattern #6166

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor Exclude by behavior into a Strategy pattern #6166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0f9c0c
Change
rlf-doordash Jul 16, 2025
64da15d
Fix lint
rlf-doordash Jul 18, 2025
fb50d36
Add removed comments
rlf-doordash Jul 18, 2025
3366cc4
PR comments
rlf-doordash Sep 3, 2025
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
15 changes: 6 additions & 9 deletions Source/SwiftLintFramework/Configuration+CommandLine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,8 @@ extension Configuration {

let scriptInputPaths = files.compactMap(\.path)

if options.useExcludingByPrefix {
return filterExcludedPathsByPrefix(in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
}
return filterExcludedPaths(excludedPaths(), in: scriptInputPaths)
let excludeBy = ExcludeByStrategyFactory.createExcludeByStrategy(options: options, configuration: self)
return excludeBy.filterExcludedPaths(in: scriptInputPaths)
.map(SwiftLintFile.init(pathDeferringReading:))
}
if !options.quiet {
Expand All @@ -272,14 +269,14 @@ extension Configuration {

queuedPrintError("\(options.capitalizedVerb) Swift files \(filesInfo)")
}
let excludeLintableFilesBy = options.useExcludingByPrefix
? Configuration.ExcludeBy.prefix
: .paths(excludedPaths: excludedPaths())

let excludeBy = ExcludeByStrategyFactory.createExcludeByStrategy(options: options, configuration: self)

return options.paths.flatMap {
self.lintableFiles(
inPath: $0,
forceExclude: options.forceExclude,
excludeBy: excludeLintableFilesBy)
excludeBy: excludeBy)
}
}

Expand Down
74 changes: 4 additions & 70 deletions Source/SwiftLintFramework/Configuration/Configuration+LintableFiles.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import Foundation

extension Configuration {
public enum ExcludeBy {
case prefix
case paths(excludedPaths: [String])
}

// MARK: Lintable Paths
/// Returns the files that can be linted by SwiftLint in the specified parent path.
///
Expand All @@ -18,7 +13,7 @@ extension Configuration {
/// - returns: Files to lint.
public func lintableFiles(inPath path: String,
forceExclude: Bool,
excludeBy: ExcludeBy) -> [SwiftLintFile] {
excludeBy: any ExcludeByStrategy) -> [SwiftLintFile] {
lintablePaths(inPath: path, forceExclude: forceExclude, excludeBy: excludeBy)
.parallelCompactMap {
SwiftLintFile(pathDeferringReading: $0)
Expand All @@ -38,17 +33,12 @@ extension Configuration {
internal func lintablePaths(
inPath path: String,
forceExclude: Bool,
excludeBy: ExcludeBy,
excludeBy: any ExcludeByStrategy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can also be some, can't it?

fileManager: some LintableFileManager = FileManager.default
) -> [String] {
if fileManager.isFile(atPath: path) {
if forceExclude {
switch excludeBy {
case .prefix:
return filterExcludedPathsByPrefix(in: [path.absolutePathStandardized()])
case .paths(let excludedPaths):
return filterExcludedPaths(excludedPaths, in: [path.absolutePathStandardized()])
}
return excludeBy.filterExcludedPaths(in: [path.absolutePathStandardized()])
}
// If path is a file and we're not forcing excludes, skip filtering with excluded/included paths
return [path]
Expand All @@ -59,62 +49,6 @@ extension Configuration {
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }

switch excludeBy {
case .prefix:
return filterExcludedPathsByPrefix(in: pathsForPath, includedPaths)
case .paths(let excludedPaths):
return filterExcludedPaths(excludedPaths, in: pathsForPath, includedPaths)
}
}

/// Returns an array of file paths after removing the excluded paths as defined by this configuration.
///
/// - parameter fileManager: The lintable file manager to use to expand the excluded paths into all matching paths.
/// - parameter paths: The input paths to filter.
///
/// - returns: The input paths after removing the excluded paths.
public func filterExcludedPaths(
_ excludedPaths: [String],
in paths: [String]...
) -> [String] {
let allPaths = paths.flatMap { $0 }
#if os(Linux)
let result = NSMutableOrderedSet(capacity: allPaths.count)
result.addObjects(from: allPaths)
#else
let result = NSMutableOrderedSet(array: allPaths)
#endif

result.minusSet(Set(excludedPaths))
// swiftlint:disable:next force_cast
return result.map { $0 as! String }
}

/// Returns the file paths that are excluded by this configuration using filtering by absolute path prefix.
///
/// For cases when excluded directories contain many lintable files (e. g. Pods) it works faster than default
/// algorithm `filterExcludedPaths`.
///
/// - returns: The input paths after removing the excluded paths.
public func filterExcludedPathsByPrefix(in paths: [String]...) -> [String] {
let allPaths = paths.flatMap { $0 }
let excludedPaths = self.excludedPaths
.parallelFlatMap { @Sendable in Glob.resolveGlob($0) }
.map { $0.absolutePathStandardized() }
return allPaths.filter { path in
!excludedPaths.contains { path.hasPrefix($0) }
}
}

/// Returns the file paths that are excluded by this configuration after expanding them using the specified file
/// manager.
///
/// - parameter fileManager: The file manager to get child paths in a given parent location.
///
/// - returns: The expanded excluded file paths.
public func excludedPaths(fileManager: some LintableFileManager = FileManager.default) -> [String] {
excludedPaths
.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: rootDirectory) }
return excludeBy.filterExcludedPaths(in: pathsForPath, includedPaths)
}
}
33 changes: 33 additions & 0 deletions Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByPathsByExpandingSubPaths.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//
// ExcludeByPathsByExpandingSubPaths.swift
//

import Foundation

public struct ExcludeByPathsByExpandingSubPaths: ExcludeByStrategy {
let excludedPaths: [String]

public init(configuration: Configuration, fileManager: some LintableFileManager = FileManager.default) {
self.excludedPaths = configuration.excludedPaths
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can call the other init.

.flatMap(Glob.resolveGlob)
.parallelFlatMap { fileManager.filesToLint(inPath: $0, rootDirectory: configuration.rootDirectory) }
}

public init(_ excludedPaths: [String]) {
self.excludedPaths = excludedPaths
}

public func filterExcludedPaths(in paths: [String]...) -> [String] {
let allPaths = paths.flatMap { $0 }
#if os(Linux)
let result = NSMutableOrderedSet(capacity: allPaths.count)
result.addObjects(from: allPaths)
#else
let result = NSMutableOrderedSet(array: allPaths)
#endif

result.minusSet(Set(excludedPaths))
// swiftlint:disable:next force_cast
return result.map { $0 as! String }
}
}
17 changes: 17 additions & 0 deletions Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByPrefixStrategy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//
// ExcludeByPrefixStrategy.swift
//

struct ExcludeByPrefixStrategy: ExcludeByStrategy {
let excludedPaths: [String]

func filterExcludedPaths(in paths: [String]...) -> [String] {
let allPaths = paths.flatMap { $0 }
let excludedPaths = self.excludedPaths
.parallelFlatMap { @Sendable in Glob.resolveGlob($0) }
.map { $0.absolutePathStandardized() }
return allPaths.filter { path in
!excludedPaths.contains { path.hasPrefix($0) }
}
}
}
7 changes: 7 additions & 0 deletions Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByStrategy.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//
// ExcludeByStrategy.swift
//

public protocol ExcludeByStrategy {
func filterExcludedPaths(in paths: [String]...) -> [String]
}
14 changes: 14 additions & 0 deletions Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByStrategyFactory.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Foundation

class ExcludeByStrategyFactory {
static func createExcludeByStrategy(options: LintOrAnalyzeOptions,
Copy link
Collaborator

@SimplyDanny SimplyDanny Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Could be part of the protocol as an extension. Otherwise, enums are typically the go-to types for factories.

Copy link
Author

Choose a reason for hiding this comment

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

What do you have in mind? Something like:

enum ExcludeByStrategyType {
    case excludeByPrefix(ExcludeByPrefixStrategy)
    case excludeByPathsByExpandingSubPaths(ExcludeByPathsByExpandingSubPaths)
    
    static func createExcludeByStrategy(options: LintOrAnalyzeOptions,
                                        configuration: Configuration,
                                        fileManager: some LintableFileManager = FileManager.default)
    -> ExcludeByStrategyType {
        if options.useExcludingByPrefix {
            let strategy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
            return .excludeByPrefix(strategy)
        }

        let strategy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
        return .excludeByPathsByExpandingSubPaths(strategy)
    }
    
    var strategy: any ExcludeByStrategy {
        switch self {
        case .excludeByPrefix(let strategy):
            return strategy
        case .excludeByPathsByExpandingSubPaths(let strategy):
            return strategy
        }
    }
}

And use it like
ExcludeByStrategyType.createExcludeByStrategy(options: options, configuration: self).strategy ?

It cannot be an extension of the protocol as it fails on call, cannot call static funcs in meta types

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what I had in mind:

protocol ExcludeByStrategy {}

struct ExcludeByPathsByExpandingSubPaths: ExcludeByStrategy {}
struct ExcludeByPrefixStrategy: ExcludeByStrategy {}

extension ExcludeByStrategy {
  static func create(_ b: Bool) -> any ExcludeByStrategy {
    if b {
      ExcludeByPathsByExpandingSubPaths()
    } else {
      ExcludeByPrefixStrategy()
    }
  }
}

configuration: Configuration,
fileManager: some LintableFileManager = FileManager.default)
-> any ExcludeByStrategy {
if options.useExcludingByPrefix {
return ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
}

return ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
}
}
52 changes: 31 additions & 21 deletions Tests/FrameworkTests/ConfigurationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,21 +288,21 @@ final class ConfigurationTests: SwiftLintTestCase {
excludedPaths: ["directory/excluded", "directory/ExcludedFile.swift"]
)

let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeBy: excludeBy,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesFile() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory/ExcludedFile.swift",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeBy: excludeBy,
fileManager: fileManager)
XCTAssertEqual([], paths)
}
Expand All @@ -311,41 +311,44 @@ final class ConfigurationTests: SwiftLintTestCase {
let fileManager = TestFileManager()
let configuration = Configuration(includedPaths: ["directory"],
excludedPaths: ["directory/ExcludedFile.swift", "directory/excluded"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)

let paths = configuration.lintablePaths(inPath: "",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeBy: excludeBy,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesDirectory() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/excluded", "directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeBy: excludeBy,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testForceExcludesDirectoryThatIsNotInExcludedButHasChildrenThatAre() {
let fileManager = TestFileManager()
let configuration = Configuration(excludedPaths: ["directory/excluded", "directory/ExcludedFile.swift"])
let excludedPaths = configuration.excludedPaths(fileManager: fileManager)
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration, fileManager: fileManager)
let paths = configuration.lintablePaths(inPath: "directory",
forceExclude: true,
excludeBy: .paths(excludedPaths: excludedPaths),
excludeBy: excludeBy,
fileManager: fileManager)
XCTAssertEqual(["directory/File1.swift", "directory/File2.swift"].absolutePathsStandardized(), paths)
}

func testLintablePaths() {
let excluded = Configuration.default.excludedPaths(fileManager: TestFileManager())
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: Configuration.default,
fileManager: TestFileManager())

let paths = Configuration.default.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: false,
excludeBy: .paths(excludedPaths: excluded))
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
let expectedFilenames = [
"DirectoryLevel1.swift",
Expand All @@ -359,9 +362,10 @@ final class ConfigurationTests: SwiftLintTestCase {
func testGlobIncludePaths() {
XCTAssert(FileManager.default.changeCurrentDirectoryPath(Mock.Dir.level0))
let configuration = Configuration(includedPaths: ["**/Level2"])
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: true,
excludeBy: .paths(excludedPaths: configuration.excludedPaths))
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
let expectedFilenames = ["Level2.swift", "Level3.swift"]

Expand All @@ -374,10 +378,10 @@ final class ConfigurationTests: SwiftLintTestCase {
excludedPaths: [Mock.Dir.level3.stringByAppendingPathComponent("*.swift")]
)

let excludedPaths = configuration.excludedPaths()
let excludeBy = ExcludeByPathsByExpandingSubPaths(configuration: configuration)
let lintablePaths = configuration.lintablePaths(inPath: "",
forceExclude: false,
excludeBy: .paths(excludedPaths: excludedPaths))
excludeBy: excludeBy)
XCTAssertEqual(lintablePaths, [])
}

Expand Down Expand Up @@ -490,29 +494,32 @@ extension ConfigurationTests {
includedPaths: ["Level1"],
excludedPaths: ["Level1/Level1.swift", "Level1/Level2/Level3"]
)
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: Mock.Dir.level0,
forceExclude: false,
excludeBy: .prefix)
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }
XCTAssertEqual(filenames, ["Level2.swift"])
}

func testExcludeByPrefixForceExcludesFile() {
XCTAssert(FileManager.default.changeCurrentDirectoryPath(Mock.Dir.level0))
let configuration = Configuration(excludedPaths: ["Level1/Level2/Level3/Level3.swift"])
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: "Level1/Level2/Level3/Level3.swift",
forceExclude: true,
excludeBy: .prefix)
excludeBy: excludeBy)
XCTAssertEqual([], paths)
}

func testExcludeByPrefixForceExcludesFileNotPresentInExcluded() {
XCTAssert(FileManager.default.changeCurrentDirectoryPath(Mock.Dir.level0))
let configuration = Configuration(includedPaths: ["Level1"],
excludedPaths: ["Level1/Level1.swift"])
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: "Level1",
forceExclude: true,
excludeBy: .prefix)
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(["Level2.swift", "Level3.swift"], filenames)
}
Expand All @@ -524,9 +531,10 @@ extension ConfigurationTests {
"Level1/Level2", "Directory.swift", "ChildConfig", "ParentConfig", "NestedConfig"
]
)
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: ".",
forceExclude: true,
excludeBy: .prefix)
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(["Level0.swift", "Level1.swift"], filenames)
}
Expand All @@ -538,9 +546,10 @@ extension ConfigurationTests {
"Level1", "Directory.swift/DirectoryLevel1.swift", "ChildConfig", "ParentConfig", "NestedConfig"
]
)
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: ".",
forceExclude: true,
excludeBy: .prefix)
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }
XCTAssertEqual(["Level0.swift"], filenames)
}
Expand All @@ -550,9 +559,10 @@ extension ConfigurationTests {
let configuration = Configuration(
includedPaths: ["Level1"],
excludedPaths: ["Level1/*/*.swift", "Level1/*/*/*.swift"])
let excludeBy = ExcludeByPrefixStrategy(excludedPaths: configuration.excludedPaths)
let paths = configuration.lintablePaths(inPath: "Level1",
forceExclude: false,
excludeBy: .prefix)
excludeBy: excludeBy)
let filenames = paths.map { $0.bridge().lastPathComponent }.sorted()
XCTAssertEqual(filenames, ["Level1.swift"])
}
Expand Down
Loading