Skip to content

improve API usages around String/Data/ByteBuffer #109

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

Merged
merged 1 commit into from
May 13, 2024
Merged

improve API usages around String/Data/ByteBuffer #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PackageDescription

let package = Package(
name: "swiftly",
platforms: [.macOS(.v13)],
products: [
.executable(
name: "swiftly",
Expand Down
2 changes: 1 addition & 1 deletion Sources/Swiftly/List.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ struct List: SwiftlyCommand {

let message = "Installed \(modifier) toolchains"
SwiftlyCore.print(message)
SwiftlyCore.print(String(repeating: "-", count: message.utf8.count))
SwiftlyCore.print(String(repeating: "-", count: message.count))
for toolchain in toolchains {
printToolchain(toolchain)
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/Swiftly/ListAvailable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ struct ListAvailable: SwiftlyCommand {

let message = "Available \(modifier) toolchains"
SwiftlyCore.print(message)
SwiftlyCore.print(String(repeating: "-", count: message.utf8.count))
SwiftlyCore.print(String(repeating: "-", count: message.count))
for toolchain in toolchains {
printToolchain(toolchain)
}
Expand Down
11 changes: 3 additions & 8 deletions Sources/SwiftlyCore/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,8 @@ public struct SwiftlyHTTPClient {

guard case .ok = response.status else {
var message = "received status \"\(response.status)\" when reaching \(url)"
if let json = response.buffer.getString(at: 0, length: response.buffer.readableBytes) {
message += ": \(json)"
}
let json = String(buffer: response.buffer)
message += ": \(json)"
throw Error(message: message)
}

Expand Down Expand Up @@ -186,9 +185,5 @@ public struct SwiftlyHTTPClient {
}

private class HTTPClientWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we go the shared route, I don't think we need this class at all actually. The original intent was to be able to share a single HTTP client via class's arc semantics and also to ensure it shut down when the application was shutting down, both of which seem to be solved by HTTPClient.shared.

If we don't go this route, then we probably still can get rid of this assuming we don't need to call syncShutdown ourselves, which it seems like we don't need to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we go the shared route, I don't think we need this class at all actually. The original intent was to be able to share a single HTTP client via class's arc semantics and also to ensure it shut down when the application was shutting down, both of which seem to be solved by HTTPClient.shared.

Right, this doesn't really work because you can't know what thread will call deinit, so if you call syncShutdown in there that can be pretty bad.

There are really two 'correct' ways:

  1. in main call: let client = HTTPClient(...); defer { try! client.syncShutdown() } and dependency inject it
  2. use a singleton. Either HTTPClient.shared or your own singleton

Most server people prefer (1) but Apple's SDKs are typically singleton-heavy and therefore the Swift community is often quite keen on simplicity which would be (2). Especially for a command-line utility like Swiftly, having a HTTPClient-singleton isn't really that bad.

Of course it's a serious design issue in HTTPClient that some of the configuration (like the proxy settings) can only be configured using the top-level object (HTTPClient) that you're really meant to share everywhere (swift-server/async-http-client#392).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think a singleton would be fine for swiftly, it's basically what we have already. Until shared can be configured to handle the proxy settings, it seems like we'll need to do it ourselves though. I think we can do that in a separate commit / PR or here if you'd like, either is fine with me.

fileprivate let inner = HTTPClient(eventLoopGroupProvider: .singleton)

deinit {
try? self.inner.syncShutdown()
}
fileprivate let inner = HTTPClient.shared
}
11 changes: 5 additions & 6 deletions Tests/SwiftlyTests/SelfUpdateTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ final class SelfUpdateTests: SwiftlyTests {
try buffer.writeJSONEncodable(nextRelease)
return HTTPClientResponse(body: .bytes(buffer))
case "github.com":
var buffer = ByteBuffer()
buffer.writeString(latestVersion)
let buffer = ByteBuffer(string: latestVersion)
return HTTPClientResponse(body: .bytes(buffer))
default:
throw SwiftlyTestError(message: "unknown url host: \(String(describing: url.host))")
Expand All @@ -43,7 +42,7 @@ final class SelfUpdateTests: SwiftlyTests {
func runSelfUpdateTest(latestVersion: String, shouldUpdate: Bool = true) async throws {
try await self.withTestHome {
let swiftlyURL = Swiftly.currentPlatform.swiftlyBinDir.appendingPathComponent("swiftly", isDirectory: false)
try "old".data(using: .utf8)!.write(to: swiftlyURL)
try Data("old".utf8).write(to: swiftlyURL)

var update = try self.parseCommand(SelfUpdate.self, ["self-update"])
update.httpClient = Self.makeMockHTTPClient(latestVersion: latestVersion)
Expand All @@ -52,9 +51,9 @@ final class SelfUpdateTests: SwiftlyTests {
let swiftly = try Data(contentsOf: swiftlyURL)

if shouldUpdate {
XCTAssertEqual(String(data: swiftly, encoding: .utf8), latestVersion)
XCTAssertEqual(String(decoding: swiftly, as: UTF8.self), latestVersion)
} else {
XCTAssertEqual(String(data: swiftly, encoding: .utf8), "old")
XCTAssertEqual(String(decoding: swiftly, as: UTF8.self), "old")
}
}
}
Expand All @@ -74,7 +73,7 @@ final class SelfUpdateTests: SwiftlyTests {
func testSelfUpdateIntegration() async throws {
try await self.withTestHome {
let swiftlyURL = Swiftly.currentPlatform.swiftlyBinDir.appendingPathComponent("swiftly", isDirectory: false)
try "old".data(using: .utf8)!.write(to: swiftlyURL)
try Data("old".utf8).write(to: swiftlyURL)

var update = try self.parseCommand(SelfUpdate.self, ["self-update"])
try await update.run()
Expand Down
2 changes: 1 addition & 1 deletion Tests/SwiftlyTests/SwiftlyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ public struct MockToolchainDownloader: HTTPRequestExecutor {
echo '\(toolchain.name)'
"""

let data = script.data(using: .utf8)!
let data = Data(script.utf8)
try data.write(to: executablePath)

// make the file executable
Expand Down
4 changes: 2 additions & 2 deletions Tests/SwiftlyTests/UseTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ final class UseTests: SwiftlyTests {
// Add an unrelated executable to the binary directory.
let existingFileName = "existing"
let existingExecutableURL = Swiftly.currentPlatform.swiftlyBinDir.appendingPathComponent(existingFileName)
let data = "hello world\n".data(using: .utf8)!
let data = Data("hello world\n".utf8)
try data.write(to: existingExecutableURL)

for (toolchain, files) in spec {
Expand Down Expand Up @@ -264,7 +264,7 @@ final class UseTests: SwiftlyTests {
let existingText = "existing"
for fileName in existingExecutables {
let existingExecutableURL = Swiftly.currentPlatform.swiftlyBinDir.appendingPathComponent(fileName)
let data = existingText.data(using: .utf8)!
let data = Data(existingText.utf8)
try data.write(to: existingExecutableURL)
}

Expand Down