Skip to content
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

Progress bar for downloads #389

Merged
merged 8 commits into from
Jul 30, 2022
Merged

Progress bar for downloads #389

merged 8 commits into from
Jul 30, 2022

Conversation

Mandera
Copy link
Contributor

@Mandera Mandera commented Jun 12, 2022

After opening #388 I thought I could try doing it myself, this is the result!

The only modified part is WDMHttpClient.get

  • Added tqdm as dependency
  • Changed requests.get's stream parameter to True
    • I saw that this was the case some versions ago, hopefully, it's alright to go back to having it enabled again
  • The progress bar is only shown if Content-Length exists in the response header and if it's greater than 100
    • Didn't want the progress bar to be shown for the "latest driver version" request which was very small
    • The value 100 should probably not be hard-coded
    • 100 might also not be the ideal default value, I only tried it on the Chrome driver
  • Download speed unit is dynamic, I used the parameters from the bytes CLI options
  • In my attempt to mess as little as possible with the other code I'm storing the content from the stream to a bytearray
  • I'm not too familiar with logging so not sure how to make the progress bar go through that if necessary
    • Found this on SO anyhow
  • Not sure how to make a unit test for this

Here's an example of how it looks
progress

Thank you

@pep8speaks
Copy link

pep8speaks commented Jun 12, 2022

Hello @Mandera! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 32:80: E501 line too long (84 > 79 characters)
Line 36:1: W391 blank line at end of file

Line 284:80: E501 line too long (81 > 79 characters)
Line 285:80: E501 line too long (96 > 79 characters)
Line 289:80: E501 line too long (114 > 79 characters)

Comment last updated at 2022-07-30 07:39:30 UTC

@Mandera Mandera changed the title Progress bar for downloading larger files Progress bar for downloads Jun 12, 2022
webdriver_manager/core/http.py Outdated Show resolved Hide resolved
Hardcoded the "[WDM] - " prefix for the progress bar as I think making tqdm use the logger's formatting dynamically isn't feasible.
Added tqdm to install_requires in setup.py (In addition to Pipfile).
@Mandera
Copy link
Contributor Author

Mandera commented Jun 26, 2022

  • Moved progress bar code to show_download_progress() in utils.py
  • Hardcoded the "[WDM] - " prefix for the progress bar as I think making tqdm use the logger's formatting dynamically isn't feasible
  • Added tqdm to install_requires in setup.py (In addition to Pipfile) - Is this the right way to do it? Seems strange to define deps in two places

@SergeyPirogov
Copy link
Owner

Well, I think that this approach shold be just documented. I dont want to add 3rd party dependency and have a progress bar enabled by default

@Mandera
Copy link
Contributor Author

Mandera commented Jun 26, 2022

Yeah makes sense to have it disabled by default, add a parameter progress_bar=False to DriverManager and its subclasses?

Are you open to making tqdm an optional dependency? Such as pip install webdriver-manager[progress-bar] or pip install webdriver-manager[full]
If not I could try writing a simplistic progress bar without 3rd party deps

@SergeyPirogov
Copy link
Owner

Yeah makes sense to have it disabled by default, add a parameter progress_bar=False to DriverManager and its subclasses?

Are you open to making tqdm an optional dependency? Such as pip install webdriver-manager[progress-bar] or pip install webdriver-manager[full] If not I could try writing a simplistic progress bar without 3rd party deps

Yes, making it optional is a great idea

@SergeyPirogov SergeyPirogov merged commit 3d4d9b2 into SergeyPirogov:master Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants