Refactor Exclude by behavior into a Strategy pattern #6166
Conversation
Here's an example of your CHANGELOG entry: * Refactor Exclude by behavior into a Strategy pattern.
[rlf-doordash](https://github.com/rlf-doordash)
[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)note: There are two invisible spaces after the entry's text. Generated by 🚫 Danger |
|
@SimplyDanny Do you happen to know how to get a review on proposed changes? |
I can have a look. But it'll take a while. |
Sounds good, would appreciate it. This is part of a larger improvement I have prepared to allow faster parsing of large codebases :D |
Source/SwiftLintFramework/Configuration/Configuration+LintableFiles.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByPathsByExpandingSubPaths.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintFramework/ExcludeByStrategy/ExcludeByPathsByExpandingSubPaths.swift
Outdated
Show resolved
Hide resolved
| import Foundation | ||
|
|
||
| class ExcludeByStrategyFactory { | ||
| static func createExcludeByStrategy(options: LintOrAnalyzeOptions, |
There was a problem hiding this comment.
Could be part of the protocol as an extension. Otherwise, enums are typically the go-to types for factories.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()
}
}
}
That motivated me come back to this PR faster. 😉 |
Sorry, I was out and it has taken a while to get back on this :( I am expectin g to be able to put some time into this for the next couple of weeks tho! |
| inPath path: String, | ||
| forceExclude: Bool, | ||
| excludeBy: ExcludeBy, | ||
| excludeBy: any ExcludeByStrategy, |
There was a problem hiding this comment.
Can also be some, can't it?
| import Foundation | ||
|
|
||
| class ExcludeByStrategyFactory { | ||
| static func createExcludeByStrategy(options: LintOrAnalyzeOptions, |
There was a problem hiding this comment.
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()
}
}
}| /// - parameter paths: The input paths to filter. | ||
| /// | ||
| /// - returns: The input paths after removing the excluded paths. | ||
| struct ExcludeByPathsByExpandingSubPaths: ExcludeByStrategy { |
There was a problem hiding this comment.
| struct ExcludeByPathsByExpandingSubPaths: ExcludeByStrategy { | |
| struct ExcludeByExpandingSubPathsStrategy: ExcludeByStrategy { |
| /// For cases when excluded directories contain many lintable files (e. g. Pods) it works faster than default | ||
| /// algorithm `filterExcludedPaths`. |
There was a problem hiding this comment.
| /// For cases when excluded directories contain many lintable files (e. g. Pods) it works faster than default | |
| /// algorithm `filterExcludedPaths`. | |
| /// For cases when excluded directories contain many lintable files (e. g. Pods) it works faster than the | |
| /// default `filterExcludedPaths` strategy. |
| let excludedPaths: [String] | ||
|
|
||
| init(configuration: Configuration, fileManager: some LintableFileManager = FileManager.default) { | ||
| self.excludedPaths = configuration.excludedPaths |
There was a problem hiding this comment.
Can call the other init.
| @@ -0,0 +1,28 @@ | |||
| import Foundation | |||
|
|
|||
| enum ExcludeByStrategyType { | |||
There was a problem hiding this comment.
The enum was only to introduce a namespace containing the factory method. We don't really need that, especially not the cases, which are sufficiently described by the two strategies conforming to the protocol.
As written in the other comment, the static factory method can be part of the protocol itself via an extension.
Right now there are only 2 ways of excluding files managed by an enum. The logic deciding between those 2 is split along several places of code, which makes it difficult to standardize between them and pose a scalability challenge if we were to add new options.
This PR creates a common protocol among those 2 options (by prefix and by expanding subpaths) and refactors them so their logic can be called generically instead of exposing inner implementation details in different places of code