-
Notifications
You must be signed in to change notification settings - Fork 143
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.
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
feat: support display download progress bar #525
Conversation
@sdankel GM sir, looking forward for your review~ |
Thanks for this! How does it look in the log output ( |
Hi @sdankel , thanks for your review. Currently, progress bar won't be wrote to log file now. As for crate "tracing-indicatif", I didn't find the way to writting progress bar into file, neither through the documents, nor from the source code. I tried some way, but failed. What would you like to see in the log file, could I make it another way, something like recording the final status about the progress bar? |
@sdankel GM sir, writting progress bar to log file has implemented. Now it will show progress bar in log file after download success or failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this out and it's looking good! It makes me think there's a lot more we could do to improve the look and feel of fuelup with spinners and multiline progress bars (if we download in parallel). Left a few comments.
downloaded_size += bytes_read as u64; | ||
progress_bar.set_position(downloaded_size); | ||
} | ||
progress_bar.finish_with_message("Download complete"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shows that the download completed even if there was an error. Could you add a unit test where it fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested in local, when download encount error, won't show "download completed"
src/download.rs
Outdated
HumanDuration(progress_bar.eta()), | ||
progress_bar.message(), | ||
); | ||
error!("Something went wrong writing data: {}", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we still log the path that it failed to write to? I think this should still be a bail!
so that we exit if the download failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
src/download.rs
Outdated
len, body, | ||
); | ||
let res = s.parse::<Response>().unwrap(); | ||
assert!(write_response_with_progress_bar(res, &mut mock_writer, String::new()).is_ok()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that we need to assert or is_ok()
here, no?
If the call fails then the panic will be triggered and the test will fail as expected, or am I misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. If we are able to catch the exact error and assert that the specific error matches what we expect in the test then we should do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. If we are able to catch the exact error and assert that the specific error matches what we expect in the test then we should do that.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
close #319
Add downloading progress bar to commands that add/install components/toolchains, now we could know wether the executable is still doing work when we met with low bandwidth/slow internet speeds.
See following pictures