feat: Add progress step component #139
Conversation
pepicrft
force-pushed
the
progress-step
branch
from
ac1936b to
eb7cf8f
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
… enhance test coverage for both scenarios
…updates; add new progress step documentation and demo GIFs
pepicrft
force-pushed
the
progress-step
branch
from
18a5ab4 to
81f794f
Compare
|
@pepicrft is this ready for review? Any reason for this being a draft? |
| with: | ||
| commitTitleMatch: false |
There was a problem hiding this comment.
I think this should be kept true – otherwise, it's easy to use the wrong message when there's just one commit
| } | ||
|
|
||
| func runNonInteractive() async throws { | ||
| /// ℹ︎ |
| let terminal: Terminaling | ||
| let renderer: Rendering | ||
| let standardPipelines: StandardPipelines | ||
| var spinner: Spinning |
| import Foundation | ||
| import Rainbow | ||
|
|
||
| class ProgressStep { |
| } | ||
| } | ||
|
|
||
| // MARK: - Private |
There was a problem hiding this comment.
The completionMessage is not private. It should be moved up. FWIW, I'm not a huge fan of the MARK sections and I'd use them sparingly. They often get out-of-date and they don't bring a ton of value (for me, anyways).
| ) { _progress in | ||
| // _progress can be used to report progress | ||
| try await doSomething() | ||
| } |
There was a problem hiding this comment.
nit: I'd add an example of how to use the progress to report progress
| standardPipelines.error | ||
| .write( | ||
| content: " \("⨯".hexIfColoredTerminal(theme.danger, terminal)) \((errorMessage ?? message).hexIfColoredTerminal(theme.muted, terminal)) \(timeString)\n" | ||
| ) |
There was a problem hiding this comment.
I wonder if static errorMessage is the best approach here.
What do you think about passing a callback in the form of (Error) -> String to transform Error into a String? But since we rethrow, it might be also fine as I'd expect the consumers to deal with that error next on their own side.
| } | ||
| } catch { | ||
| _error = error | ||
| } | ||
|
|
||
| let elapsedTime = Double(DispatchTime.now().uptimeNanoseconds - start.uptimeNanoseconds) / 1_000_000_000 | ||
| let timeString = "[\(String(format: "%.1f", elapsedTime))s]".hexIfColoredTerminal(theme.muted, terminal) | ||
|
|
||
| if _error != nil { | ||
| standardPipelines.error | ||
| .write( | ||
| content: " \("⨯".hexIfColoredTerminal(theme.danger, terminal)) \((errorMessage ?? message).hexIfColoredTerminal(theme.muted, terminal)) \(timeString)\n" | ||
| ) | ||
| } else { | ||
| let message = ProgressStep | ||
| .completionMessage(successMessage ?? message, timeString: timeString, theme: theme, terminal: terminal) | ||
| standardPipelines.output.write(content: " \(message)\n") | ||
| } | ||
|
|
||
| // swiftlint:disable:next identifier_name | ||
| if let _error { | ||
| throw _error | ||
| } |
There was a problem hiding this comment.
I'd extract the following two lines into a helped method/closure:
private func timeString(start: DispatchTime) -> String {
let elapsedTime = Double(DispatchTime.now().uptimeNanoseconds - start.uptimeNanoseconds) / 1_000_000_000
return "[\(String(format: "%.1f", elapsedTime))s]".hexIfColoredTerminal(theme.muted, terminal)
}
That way, we can save ourselves some of the dancing with _error
| } | |
| } catch { | |
| _error = error | |
| } | |
| let elapsedTime = Double(DispatchTime.now().uptimeNanoseconds - start.uptimeNanoseconds) / 1_000_000_000 | |
| let timeString = "[\(String(format: "%.1f", elapsedTime))s]".hexIfColoredTerminal(theme.muted, terminal) | |
| if _error != nil { | |
| standardPipelines.error | |
| .write( | |
| content: " \("⨯".hexIfColoredTerminal(theme.danger, terminal)) \((errorMessage ?? message).hexIfColoredTerminal(theme.muted, terminal)) \(timeString)\n" | |
| ) | |
| } else { | |
| let message = ProgressStep | |
| .completionMessage(successMessage ?? message, timeString: timeString, theme: theme, terminal: terminal) | |
| standardPipelines.output.write(content: " \(message)\n") | |
| } | |
| // swiftlint:disable:next identifier_name | |
| if let _error { | |
| throw _error | |
| } | |
| } | |
| let message = ProgressStep | |
| .completionMessage(successMessage ?? message, timeString: timeString(start: start), theme: theme, terminal: terminal) | |
| standardPipelines.output.write(content: " \(message)\n") | |
| } catch { | |
| standardPipelines.error | |
| .write( | |
| content: " \("⨯".hexIfColoredTerminal(theme.danger, terminal)) \((errorMessage ?? message).hexIfColoredTerminal(theme.muted, terminal)) \(timeString(start: start))\n" | |
| ) | |
| throw error | |
| } |
There was a problem hiding this comment.
The same goes for runInteractive – we can make the code more readable if we skip doing the _error dance.
| // MARK: - Private | ||
|
|
||
| static func completionMessage(_ message: String, timeString: String? = nil, theme: Theme, terminal: Terminaling) -> String { | ||
| "\("✔︎".hexIfColoredTerminal(theme.success, terminal)) \(message)\(timeString != nil ? " \(timeString!)" : "")" |
There was a problem hiding this comment.
| "\("✔︎".hexIfColoredTerminal(theme.success, terminal)) \(message)\(timeString != nil ? " \(timeString!)" : "")" | |
| "\("✔︎".hexIfColoredTerminal(theme.success, terminal)) \(message)\(timeString ?? "")" |
| message: String, | ||
| successMessage: String?, | ||
| errorMessage: String?, | ||
| showSpinner: Bool, | ||
| action: @escaping (@escaping (String) -> Void) async throws -> Void, | ||
| theme: Theme, |
There was a problem hiding this comment.
I'd keep the init for dependency injection and pass the rest of the variables through the run invocation
TL;DR
Added a new progress step component to display step-by-step execution with timing and status indicators.
What changed?
ProgressStepcomponent that shows execution progress with optional spinnerHow to test?
Why make this change?
To provide a consistent way to display step-by-step progress during command execution with visual feedback, timing information, and proper error handling. This improves the user experience by giving clear visibility into long-running operations.