refactor: progress bar#1914
Conversation
ec8d326 to
906c708
Compare
Not refreshing the bar when no progress happens, improves usability when awaiting user input from stdin. (`lychee -`) Also add a debugging statement as users might wonder why lychee blocks.
c70f5f5 to
1bcdb18
Compare
The only resulting difference is that the progress bar message now shows the response detailed, which only affects erroneous link check results. So in practice the difference is barely noticeable while reducing complexity qute a bit.
Summary@mre I realised that not showing the progress bar as requested in #1736 is not feasible. What bothered me the most, is that the progress bar was clogging up stdin when waiting for user input with So I mostly explored 2 ideas when working on this PR.
As the link collection and checking is so highly async it means that we had to introduce some special handling for stdin. We would have to make sure that the stdin element would always be the first input that is checked. This would require a huge effort for such a minimal issue as everything is so async and it would increase complexity in the codebase. For example running With this in mind I thought about the second approach. But this doesn't work either because of the concurrency. We would basically have to hide the progress as with This is why I came up with the simple solution of removing I've spent more time on this than I should admit but I've also refactored and simplified the code around |
Sorry, I don't quite understand what you mean by "thread in the background." 😅 What is the architectural advantage of that approach? One thread less I guess? But how does it avoid cluttering the terminal?
Thanks for that! It looks much cleaner. One more rebase and we can merge this I guess. Good work! |
It's not really about an architectural advantage (though one thread less is nice as well) but about UX. Try running |
In #1914, work was done to hide the progress bar on `stdin`. The simple solution of removing enable_steady_tick for the progress bar. This worked well for avoiding cluttering the output while the user typed their input. However, the initial progress bar still gets shown, which is not ideal. This commit slightly imporves the behavior by waiting for all inputs before showing the progress bar. We do this by iterating over the inputs and checking if any of them is `stdin`. This way, the progress bar does no longer get shown. To start the processing, the user has to press Ctrl^D, which is customs for other Unix tools as well. Other behavior remains unchanged.
In #1914, work was done to hide the progress bar on `stdin`. The simple solution of removing enable_steady_tick for the progress bar. This worked well for avoiding cluttering the output while the user typed their input. However, the initial progress bar still gets shown, which is not ideal. This commit slightly imporves the behavior by waiting for all inputs before showing the progress bar. We do this by iterating over the inputs and checking if any of them is `stdin`. This way, the progress bar does no longer get shown. To start the processing, the user has to press Ctrl^D, which is customs for other Unix tools as well. Other behavior remains unchanged.
Clone of #1835 with one additional commit with the review comments resolved. Then the main work - more refactoring. The PR retains the current behaviour as much as possible. The only notable behavioural difference is that the bar no longer uses
enable_steady_tickand the spinner animation has been removed as described in the summary comment.Closes #1736