Feat: Add progress bar #163
Conversation
|
Hi @pepicrft, can you check if I’m on the right track? This is my first time creating a CLI, and I’m really excited to get your feedback |
Wow! This is amazing @M7md-Ebrahim. Thanks a lot for taking the lead here. There's no deadline here, so no rush, it's important that you take your time to land a great first contribution and get familiar with all the CLI concepts and ideas. |
There was a problem hiding this comment.
The animations looks amazing, great job!
I have two minor comments on the style of the animation:
- The labels and the loading blocks should have vertical center alignment
- The percentage label should be filled with spaces if less than one hundred, so the pipe
|doesn't move as the percentage grows.
The implementation overall looks good to me – let's add some tests and documentation and I'd be happy to take another look.
Also, sorry for the PR review having taken a bit longer here. We'll try to do a better job at that, so you get a faster turnaround time 🙂
|
|
||
| class CLIProgressBar { |
There was a problem hiding this comment.
| class CLIProgressBar { | |
| struct CLIProgressBar { |
We prefer using structs whenever possible. Let's use classes only when you need in-place mutability.
| @@ -111,6 +111,15 @@ public protocol Noorable { | |||
| /// - Parameters: | |||
| /// - alerts: The warning messages. | |||
| func warning(_ alerts: WarningAlert...) | |||
|
|
|||
| /// | |||
There was a problem hiding this comment.
Let’s leave the docs and tests for the end, after finishing the tasks
What do you think?
| func startProgress(total: Int, interval: TimeInterval?, block: @escaping (String, Int) -> Void) | ||
| func stop() | ||
| } | ||
| class DefaultProgressBar: ProgressBar { |
There was a problem hiding this comment.
nit:
| class DefaultProgressBar: ProgressBar { | |
| final class DefaultProgressBar: ProgressBar { |
|
@fortmarek Thanks for the great review! I suggest setting the label or message length to a maximum of 20 characters. If the percentage label is less than 10, add two spaces for alignment, if it’s less than 100, add a single space. I’ll work on implementing this. Additionally, I’m researching how to calculate the time required for this task to include an ETA. |
Yeah, we can trim it with the
This sounds like a nice improvement, but let's add that in a subsequent PR 🙏 |
There was a problem hiding this comment.
Thanks a lot for this @M7md-Ebrahim. Let me know if you need any help to push this to a finish line.
| import Rainbow | ||
|
|
||
|
|
||
| struct CLIProgressBar { |
There was a problem hiding this comment.
We are not prefixing the component names with CLI so I'd stay consistent to that and rename this one to:
| struct CLIProgressBar { | |
| struct ProgressBar { |
| message: "Loading manifests", | ||
| successMessage: "Manifests loaded", | ||
| errorMessage: "Failed to load manifests", | ||
| total: 100 |
There was a problem hiding this comment.
I'd drop this argument. What are the scenarios where you don't want the progress bar to reach 100%?
| successMessage: "Xcode projects and workspace generated", | ||
| errorMessage: "Failed to generate Xcode workspace and projects", | ||
| total: 200 | ||
| ) { _ in |
There was a problem hiding this comment.
The task should have an API to control the progress. Something along the lines of:
progressBar(....) { progress in
try await doSomethingThatTakesTime { percentage in // e.g. downloading a file
progress(percentage)
}
}There was a problem hiding this comment.
Do you want to separate the calculation of the progress bar task, can you explain more? i found this method in the docs
| protocol ProgressBar { | ||
| func startProgress(total: Int, interval: TimeInterval?, block: @escaping (String, Int) -> Void) | ||
| func stop() | ||
| } | ||
| final class DefaultProgressBar: ProgressBar { |
There was a problem hiding this comment.
I'd merge all of this into the ProgressBar component to minimise the number of files making the codebase easier to navigate. If any logic from the component is reusable in the future, we can then make the decision to extract it.
There was a problem hiding this comment.
Okay, but I think it would be beneficial, as the progress bar still needs many improvements.
| progressBar.startProgress(total: total, interval: 0.05) { progressBarState, percentage in | ||
| bar = progressBarState | ||
| progressPercentage = percentage | ||
| self.render(lastMessage, bar, progressPercentage) | ||
| } |
There was a problem hiding this comment.
As mentioned in another comment, it shouldn't be the controller the one that controls the progress, but the caller that uses the component to run an asynchronous action.
This PR introduces an initial implementation of a Progress Bar Component #154
Tasks:
Notes: