Add progress reporter protocol #394
Conversation
roulpriya
requested review from
cmcgee1024,
etcwilde,
justice-adams-apple and
shahmishal
as code owners
|
|
||
| public protocol ProgressReporterProtocol { | ||
| /// Updates the progress animation with the current step, total steps, and an optional text message. | ||
| func update(step: Int, total: Int, text: String) async |
There was a problem hiding this comment.
suggestion: Since update can be fallible due to one of the implementations doing I/O operations, this can be declared as throws and then the try? become plain try and the original I/O error can flow all of the way to the top-level error reporting.
| func update(step: Int, total: Int, text: String) async | ||
|
|
||
| /// Completes the progress animation, indicating success or failure. | ||
| func complete(success: Bool) async |
There was a problem hiding this comment.
suggestion: Same as above, this can be declared throws so that I/O errors reported on the complete can be thrown to an error reported with the original I/O error details.
| } | ||
|
|
||
| private func writeProgress(_ progress: ProgressInfo) async { | ||
| let jsonData = try? self.encoder.encode(progress) |
There was a problem hiding this comment.
suggestion: From above, if this method is declared throws then the errors can be thrown directly to the caller with the particular details of the failed I/O operation.
| private func writeProgress(_ progress: ProgressInfo) async { | ||
| let jsonData = try? self.encoder.encode(progress) | ||
| guard let jsonData = jsonData else { | ||
| await self.ctx.message("Failed to encode progress entry to JSON") |
There was a problem hiding this comment.
praise: It's great to see this message being reliably reported now that the method is async.
| import SwiftlyCore | ||
| import SystemPackage | ||
| @preconcurrency import TSCBasic | ||
| import TSCUtility |
There was a problem hiding this comment.
praise: It's great to see this TSC dependency being further isolated. The hope is to remove the dependency entirely someday, and use something that isn't deprecated, and adds to the size of the swiftly binary.
| message: "\(version) does not exist at URL \(notFound.url), exiting") | ||
| } catch { | ||
| animation.complete(success: false) | ||
| try? await animation.complete(success: false) |
There was a problem hiding this comment.
thought: It's good to do a best effort complete here so that the original error gets reported since that's probably the more relevant one.
There was a problem hiding this comment.
thought: It's good to do a best effort complete here so that the original error gets reported, since that's probably the more relevant one.
I didn't get this @cmcgee1024.
There was a problem hiding this comment.
@roulpriya in this catch block, there's an error that was thrown. The animation.complete() can also throw an error. I think that the first error is probably more important and should be the one that is thrown. This code is doing the right thing in my opinion. :+1
There was a problem hiding this comment.
@roulpriya in this catch block, there's an error that was thrown. The animation.complete() can also throw an error. I think that the first error is probably more important and should be the one that is thrown. This code is doing the right thing in my opinion.
Sources/Swiftly/Install.swift
Outdated
| "Downloaded \(String(format: "%.1f", downloadedMiB)) MiB of \(String(format: "%.1f", totalMiB)) MiB" | ||
| ) | ||
| } catch { | ||
| // Silently ignore progress update errors to avoid interrupting download |
There was a problem hiding this comment.
suggestion (blocking): Let's at least report a message about this error so that it can be seen by someone or captured as part of stderr with a ctx.message(). It could have important details in VSCode integration someday.
roulpriya
force-pushed
the
add-progress-reporter-protocol
branch
from
d4ea316 to
db55485
Compare
roulpriya
force-pushed
the
add-progress-reporter-protocol
branch
from
db55485 to
af7b2d9
Compare
2 participants
This PR contains