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

Remove progressbar from pooch fetch and downloads API #3693

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Dec 6, 2022

As pointed out in a question on Stack Overflow, PyVista breaks on anything older than the currently newest pooch (1.6.0, released in January 2022).

Removing the progressbar kwarg from the call to fetch() makes us compatible with pooch 1.3.0 the oldest, which was released 2 years ago. 1.3.0 added the retry_if_failed feature, which is something we should keep, so this should be an acceptable compromise.

This is a minor breaking change if anyone downstream used the progress_bar kwarg of downloads.

@adeak adeak added the bug Uh-oh! Something isn't working as expected. label Dec 6, 2022
tkoyama010
tkoyama010 previously approved these changes Dec 6, 2022
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

@adeak
Copy link
Member Author

adeak commented Dec 6, 2022

Almost forgot to remove the functionality from the downloads API 😅

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #3693 (3710e3a) into main (f5dbce8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3693   +/-   ##
=======================================
  Coverage   94.05%   94.05%           
=======================================
  Files          83       83           
  Lines       18558    18558           
=======================================
  Hits        17454    17454           
  Misses       1104     1104           

Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this as is and don't think we need to catch/deprecate the user's use of progress_bar argument

@banesullivan
Copy link
Member

Thanks, @adeak!!

@adeak
Copy link
Member Author

adeak commented Dec 7, 2022

I'm happy with this as is and don't think we need to catch/deprecate the user's use of progress_bar argument

Good point; let me label this as a breaking change though.

@adeak adeak added the breaking-change Changes that break backwards compatibility, especially changed default parameters. label Dec 7, 2022
@adeak adeak changed the title Remove progressbar from pooch fetch Remove progressbar from pooch fetch and downloads API Dec 7, 2022
@tkoyama010 tkoyama010 merged commit cb963fa into pyvista:main Dec 9, 2022
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that break backwards compatibility, especially changed default parameters. bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants