Refactor progress bar#1835
Conversation
Print parameter is unnecessary. When the log level is set to info or higher: - we don’t use the progress bar while checking links. - we do use the progress bar when suggesting alternatives, but that code path does not invoke update.
- Introduced `LycheeProgressBarConfig` to hold all progress‑bar settings (template, increment, tick interval, progress characters) instead of scattering them as associated constants inside `LycheeProgressBar`. - The wrapper now stores a `cfg: LycheeProgressBarConfig` instance. - Added a `Default` implementation for `LycheeProgrssBarConfig` so existing callers continue to work unchanged.
The function can be replaced with set_length. It also doesn't need to print request next to the bar
fdc3ea7 to
127fcf8
Compare
127fcf8 to
7414d7e
Compare
| let request = request?; | ||
| if let Some(pb) = &bar { | ||
| pb.inc_length(1); | ||
| pb.set_message(request.to_string()); |
There was a problem hiding this comment.
i don't think we need to set msg here?
There was a problem hiding this comment.
We should keep it IMO because showing the extracted links and not only the link after having checked it, makes sense in my opinion.
thomas-zahner
left a comment
There was a problem hiding this comment.
Thanks for the PR. Extracting the progress bar into a new struct and file does make sense and improves readability and maintainability 👍
| let request = request?; | ||
| if let Some(pb) = &bar { | ||
| pb.inc_length(1); | ||
| pb.set_message(request.to_string()); |
There was a problem hiding this comment.
We should keep it IMO because showing the extracted links and not only the link after having checked it, makes sense in my opinion.
| let mut request_count = 0; | ||
| while let Some(request) = requests.next().await { | ||
| let request = request?; | ||
| if let Some(pb) = &bar { | ||
| pb.inc_length(1); | ||
| pb.set_message(request.to_string()); | ||
| } | ||
| request_count += 1; | ||
| send_req | ||
| .send(Ok(request)) | ||
| .await | ||
| .expect("Cannot send request"); | ||
| } | ||
| if let Some(pb) = &bar { | ||
| pb.set_length(request_count); | ||
| } |
There was a problem hiding this comment.
The original version of this section made more sense. Extracting the links from the inputs (and awaiting the requests, before executing the requests) and displaying the result after having executed the request are two independent highly concurrent processes. Because of that the progress bar size grows, even as we are already performing requests. So setting the progress bar length only after all requests were created makes no sense.
Run cargo run fixtures/ with your PR version and you will see, how the progress bar remains at 0 during the whole link check process, displaying something like 2114/0.
| pub(crate) fn update(&self, message: Option<String>) { | ||
| self.bar.inc(self.config.increment); | ||
| if let Some(msg) = message { | ||
| self.bar.set_message(msg.clone()); |
There was a problem hiding this comment.
I don't think we need to clone here
|
@thomas-zahner thanks a lot for reviewing this. I'm going to address the comments as soon as I can. I'm a bit busy these days, so sorry for the late response. |
| } | ||
|
|
||
| #[derive(Clone)] | ||
| pub(crate) struct LycheeProgressBar { |
There was a problem hiding this comment.
Can we call it ProgressBar instead? I think the Lychee is sort of implied. 😉 Same for the filename.
|
@rodic any updates? |
|
I'm sorry @mre i'll try to get it done this week. |
|
Ah no worries and no rush. Just wanted to avoid that this awesome work goes stale. |
|
i apologize again for being late, i didn't forget about it :) |
Refactor of progress‑bar
LycheeProgressBar– a thin wrapper around theindicatiflibrary that centralises all progress‑bar logic (configuration, styling, ticking, length handling, and finishing).ProgressBarwith the new wrapper, improving readability and making future UI changes easier to manage.LycheeProgressBarConfig) with sensible defaults (template, increment step, tick interval, progress characters).