package collections small refactoring (#3072)

motivation: nicer code

changes:
* create an internal alias for the model defintion so code is less verbose
* better handling of default http client initialization

Author: tomerd

Sources/PackageCollections/Model/Collection.swift

@@ -17,6 +17,9 @@ import TSCUtility
 
 public enum PackageCollectionsModel {}
 
+// make things less verbose internally
+internal typealias Model = PackageCollectionsModel
+
 extension PackageCollectionsModel {
     /// A `Collection` is a collection of packages.
     public struct Collection: Equatable, Codable {
@@ -46,7 +49,7 @@ extension PackageCollectionsModel {
 
         /// Who authored this collection
         public let createdBy: Author?
-        
+
         /// When this collection was last processed locally
         public let lastProcessedAt: Date
 

Sources/PackageCollections/Model/Package.swift

@@ -112,7 +112,7 @@ extension PackageCollectionsModel.Package {
 
         /// The package version's Swift tools version
         public let toolsVersion: ToolsVersion
-        
+
         /// The package version's supported platforms
         public let minimumPlatformVersions: [SupportedPlatform]?
 

Sources/PackageCollections/PackageCollections+Validation.swift

@@ -10,7 +10,7 @@
 
 import TSCBasic
 
-extension PackageCollectionsModel.CollectionSource {
+extension Model.CollectionSource {
     func validate() -> [ValidationError]? {
         var errors: [ValidationError]?
         let appendError = { (error: ValidationError) in

Sources/PackageCollections/PackageCollections.swift

@@ -15,7 +15,7 @@ import TSCBasic
 public struct PackageCollections: PackageCollectionsProtocol {
     private let configuration: Configuration
     private let storageContainer: (storage: Storage, owned: Bool)
-    private let collectionProviders: [PackageCollectionsModel.CollectionSourceType: PackageCollectionProvider]
+    private let collectionProviders: [Model.CollectionSourceType: PackageCollectionProvider]
     private let metadataProvider: PackageMetadataProvider
 
     private var storage: Storage {
@@ -26,7 +26,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
     public init(configuration: Configuration = .init()) {
         let storage = Storage(sources: FilePackageCollectionsSourcesStorage(),
                               collections: SQLitePackageCollectionsStorage())
-        let collectionProviders = [PackageCollectionsModel.CollectionSourceType.json: JSONPackageCollectionProvider()]
+        let collectionProviders = [Model.CollectionSourceType.json: JSONPackageCollectionProvider()]
         let metadataProvider = GitHubPackageMetadataProvider()
 
         self.configuration = configuration
@@ -38,7 +38,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
     // internal initializer for testing
     init(configuration: Configuration = .init(),
          storage: Storage,
-         collectionProviders: [PackageCollectionsModel.CollectionSourceType: PackageCollectionProvider],
+         collectionProviders: [Model.CollectionSourceType: PackageCollectionProvider],
          metadataProvider: PackageMetadataProvider) {
         self.configuration = configuration
         self.storageContainer = (storage, false)
@@ -65,7 +65,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
                 if identifiers.isEmpty {
                     return callback(.success([]))
                 }
-                let collectionOrder = identifiers.enumerated().reduce([PackageCollectionsModel.CollectionIdentifier: Int]()) { partial, element in
+                let collectionOrder = identifiers.enumerated().reduce([Model.CollectionIdentifier: Int]()) { partial, element in
                     var dictionary = partial
                     dictionary[element.element] = element.offset
                     return dictionary
@@ -94,7 +94,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
                     return callback(.success([]))
                 }
                 let lock = Lock()
-                var refreshResults = [Result<PackageCollectionsModel.Collection, Error>]()
+                var refreshResults = [Result<Model.Collection, Error>]()
                 sources.forEach { source in
                     self.refreshCollectionFromSource(source: source) { refreshResult in
                         lock.withLock { refreshResults.append(refreshResult) }
@@ -181,7 +181,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
             case .success(let sources):
                 let identifiers = sources.map { .init(from: $0) }.filter { collections?.contains($0) ?? true }
                 if identifiers.isEmpty {
-                    return callback(.success(PackageCollectionsModel.PackageSearchResult(items: [])))
+                    return callback(.success(Model.PackageSearchResult(items: [])))
                 }
                 self.storage.collections.searchPackages(identifiers: identifiers, query: query, callback: callback)
             }
@@ -202,7 +202,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
                 self.metadataProvider.get(reference) { result in
                     switch result {
                     case .failure(let error) where error is NotFoundError:
-                        let metadata = PackageCollectionsModel.PackageMetadata(
+                        let metadata = Model.PackageMetadata(
                             package: Self.mergedPackageMetadata(package: packageSearchResult.package, basicMetadata: nil),
                             collections: packageSearchResult.collections
                         )
@@ -211,7 +211,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
                         callback(.failure(error))
                     case .success(let basicMetadata):
                         // finally merge the results
-                        let metadata = PackageCollectionsModel.PackageMetadata(
+                        let metadata = Model.PackageMetadata(
                             package: Self.mergedPackageMetadata(package: packageSearchResult.package, basicMetadata: basicMetadata),
                             collections: packageSearchResult.collections
                         )
@@ -267,7 +267,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
     // This helps avoid network access in normal operations
     private func refreshCollectionFromSource(source: PackageCollectionsModel.CollectionSource,
                                              order _: Int? = nil,
-                                             callback: @escaping (Result<PackageCollectionsModel.Collection, Error>) -> Void) {
+                                             callback: @escaping (Result<Model.Collection, Error>) -> Void) {
         if let errors = source.validate() {
             return callback(.failure(MultipleErrors(errors)))
         }
@@ -293,7 +293,7 @@ public struct PackageCollections: PackageCollectionsProtocol {
             case .failure(let error):
                 callback(.failure(error))
             case .success(let sources):
-                let identifiers = sources.map { PackageCollectionsModel.CollectionIdentifier(from: $0) }
+                let identifiers = sources.map { Model.CollectionIdentifier(from: $0) }
                 if identifiers.isEmpty {
                     return callback(.failure(NotFoundError("\(identifier)")))
                 }
@@ -302,9 +302,9 @@ public struct PackageCollections: PackageCollectionsProtocol {
         }
     }
 
-    private func targetListResultFromCollections(_ collections: [PackageCollectionsModel.Collection]) -> PackageCollectionsModel.TargetListResult {
-        var packageCollections = [PackageReference: (package: PackageCollectionsModel.Package, collections: Set<PackageCollectionsModel.CollectionIdentifier>)]()
-        var targetsPackages = [String: (target: PackageCollectionsModel.Target, packages: Set<PackageReference>)]()
+    private func targetListResultFromCollections(_ collections: [Model.Collection]) -> Model.TargetListResult {
+        var packageCollections = [PackageReference: (package: Model.Package, collections: Set<Model.CollectionIdentifier>)]()
+        var targetsPackages = [String: (target: Model.Target, packages: Set<PackageReference>)]()
 
         collections.forEach { collection in
             collection.packages.forEach { package in
@@ -327,21 +327,21 @@ public struct PackageCollections: PackageCollectionsProtocol {
         return targetsPackages.map { _, pair in
             let targetPackages = pair.packages
                 .compactMap { packageCollections[$0] }
-                .map { pair -> PackageCollectionsModel.TargetListResult.Package in
-                    let versions = pair.package.versions.map { PackageCollectionsModel.TargetListResult.PackageVersion(version: $0.version, packageName: $0.packageName) }
+                .map { pair -> Model.TargetListResult.Package in
+                    let versions = pair.package.versions.map { Model.TargetListResult.PackageVersion(version: $0.version, packageName: $0.packageName) }
                     return .init(repository: pair.package.repository,
                                  summary: pair.package.summary,
                                  versions: versions,
                                  collections: Array(pair.collections))
                 }
 
-            return PackageCollectionsModel.TargetListItem(target: pair.target, packages: targetPackages)
+            return Model.TargetListItem(target: pair.target, packages: targetPackages)
         }
     }
 
-    internal static func mergedPackageMetadata(package: PackageCollectionsModel.Package,
-                                               basicMetadata: PackageCollectionsModel.PackageBasicMetadata?) -> PackageCollectionsModel.Package {
-        var versions = package.versions.map { packageVersion -> PackageCollectionsModel.Package.Version in
+    internal static func mergedPackageMetadata(package: Model.Package,
+                                               basicMetadata: Model.PackageBasicMetadata?) -> Model.Package {
+        var versions = package.versions.map { packageVersion -> Model.Package.Version in
             .init(version: packageVersion.version,
                   packageName: packageVersion.packageName,
                   targets: packageVersion.targets,
@@ -371,9 +371,9 @@ public struct PackageCollections: PackageCollectionsProtocol {
 }
 
 private struct UnknownProvider: Error {
-    let sourceType: PackageCollectionsModel.CollectionSourceType
+    let sourceType: Model.CollectionSourceType
 
-    init(_ sourceType: PackageCollectionsModel.CollectionSourceType) {
+    init(_ sourceType: Model.CollectionSourceType) {
         self.sourceType = sourceType
     }
 }

Sources/PackageCollections/Providers/GitHubPackageMetadataProvider.swift

@@ -20,13 +20,11 @@ import TSCBasic
 
 struct GitHubPackageMetadataProvider: PackageMetadataProvider {
     let httpClient: HTTPClient
-    let defaultHttpClient: Bool
     let decoder: JSONDecoder
     let queue: DispatchQueue
 
     init(httpClient: HTTPClient? = nil) {
-        self.httpClient = httpClient ?? .init()
-        self.defaultHttpClient = httpClient == nil
+        self.httpClient = httpClient ?? Self.makeDefaultHTTPClient()
         self.decoder = JSONDecoder()
         #if os(Linux) || os(Windows)
         self.decoder.dateDecodingStrategy = .iso8601
@@ -40,7 +38,7 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
         self.queue = DispatchQueue(label: "org.swift.swiftpm.GitHubPackageMetadataProvider", attributes: .concurrent)
     }
 
-    func get(_ reference: PackageReference, callback: @escaping (Result<PackageCollectionsModel.PackageBasicMetadata, Error>) -> Void) {
+    func get(_ reference: PackageReference, callback: @escaping (Result<Model.PackageBasicMetadata, Error>) -> Void) {
         guard reference.kind == .remote else {
             return callback(.failure(Errors.invalidReferenceType(reference)))
         }
@@ -143,19 +141,18 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
         var options = HTTPClientRequest.Options()
         options.addUserAgent = true
         options.validResponseCodes = validResponseCodes
-        if defaultHttpClient {
-            // TODO: make these defaults configurable?
-            options.timeout = httpClient.configuration.requestTimeout ?? .seconds(1)
-            options.retryStrategy = httpClient.configuration.retryStrategy ?? .exponentialBackoff(maxAttempts: 3, baseDelay: .milliseconds(50))
-            options.circuitBreakerStrategy = httpClient.configuration.circuitBreakerStrategy ?? .hostErrors(maxErrors: 5, age: .seconds(5))
-        } else {
-            options.timeout = httpClient.configuration.requestTimeout
-            options.retryStrategy = httpClient.configuration.retryStrategy
-            options.circuitBreakerStrategy = httpClient.configuration.circuitBreakerStrategy
-        }
         return options
     }
 
+    private static func makeDefaultHTTPClient() -> HTTPClient {
+        var client = HTTPClient()
+        // TODO: make these defaults configurable?
+        client.configuration.requestTimeout = .seconds(1)
+        client.configuration.retryStrategy = .exponentialBackoff(maxAttempts: 3, baseDelay: .milliseconds(50))
+        client.configuration.circuitBreakerStrategy = .hostErrors(maxErrors: 50, age: .seconds(30))
+        return client
+    }
+
     enum Errors: Error, Equatable {
         case invalidReferenceType(PackageReference)
         case invalidGitUrl(String)