Implement update command #67
Conversation
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| /// HTTPClient wrapper used for interfacing with various APIs and downloading things. | ||
| public class HTTP { | ||
| public class SwiftlyHTTPClient { |
There was a problem hiding this comment.
Most of swiftly's test's runtime is devoted to downloading toolchains, and so in an effort to reduce that, I introduced the ability to mock toolchain downloads. This allows the update tests to run quickly while still verifying that updating works. We can consider expanding the use of mocking to other tests to reduce how long they run for, though I do think there is value in having the install command tests be non-mocked.
There was a problem hiding this comment.
Can we add some comment above ToolchainDownloader detailing why it is needed
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| /// HTTPClient wrapper used for interfacing with various APIs and downloading things. | ||
| public class HTTP { | ||
| public class SwiftlyHTTPClient { | ||
| private static let client = HTTPClientWrapper() |
There was a problem hiding this comment.
You create a new HTTPClient with every instance of SwiftlyHTTPClient. Perhaps HTTPClientWrapper should be a wrapper for a global instance.
There was a problem hiding this comment.
That's how it used to be, but it was changed to be this way so that tests could construct individual commands with their own mocked HTTP clients. The lifetime of a swiftly process is only a single command in actual usage, so having them per-command is essentially the same as a singleton in practice.
| } | ||
|
|
||
| /// Use a toolchain. This method modifies and saves the input config. | ||
| internal static func execute(_ toolchain: ToolchainVersion, config: inout Config) async throws { | ||
| internal static func execute(_ toolchain: ToolchainVersion, _ config: inout Config) async throws { |
There was a problem hiding this comment.
Any reason you removed the config label from all the execute functions?
There was a problem hiding this comment.
The config variables being passed in were almost always named config themselves, so the call sites looked a little duplicative.
|
|
||
| public var httpClient = SwiftlyHTTPClient() | ||
|
|
||
| private enum CodingKeys: String, CodingKey { |
There was a problem hiding this comment.
What are the coding keys for?
There was a problem hiding this comment.
Decodable is a requirement of AsyncParsableCommand, which Update implements through SwiftlyCommand. The HTTP client isn't something that's decoded from the user input though, so I needed to introduce the coding keys to inform the Swift compiler that it should be skipped when autogenerating the Decodable conformance.
Sources/Swiftly/Update.swift
Outdated
| release.major == oldRelease.major | ||
| && release.minor == oldRelease.minor | ||
| && release.patch > oldRelease.patch | ||
| private func fetchNewToolchain(_ bounds: UpdateParameters) async throws -> ToolchainVersion? { |
There was a problem hiding this comment.
Can we function header comments here and on other functions. Is this downloading the toolchain?
There was a problem hiding this comment.
Done. This just queries the API to see if there is a toolchain that meets the requirements (e.g. if we're trying to update 5.0.0 to the latest patch version, is there one? If so, return it). I renamed this to lookupNewToolchain to better reflect that.
| } | ||
| } | ||
|
|
||
| enum UpdateParameters { |
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| /// HTTPClient wrapper used for interfacing with various APIs and downloading things. | ||
| public class HTTP { | ||
| public class SwiftlyHTTPClient { |
There was a problem hiding this comment.
Can we add some comment above ToolchainDownloader detailing why it is needed
Sources/SwiftlyCore/HTTPClient.swift
Outdated
| reportProgress: reportProgress | ||
| ) | ||
| } else { | ||
| let fileHandle = try FileHandle(forWritingTo: URL(fileURLWithPath: destination)) |
There was a problem hiding this comment.
Why is this not implemented as a ToolchainDownloader
There was a problem hiding this comment.
No good reason in particular, updated to be.
Closes #5
See the README or the
--helpoutput inUpdate.swiftfor information on the semantics of the command.