Cadair / parfive

An asyncio based parallel file downloader for Python 3.8+
https://parfive.readthedocs.io/
MIT License
51 stars 24 forks source link

Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Closed Cadair closed 2 years ago

Cadair commented 2 years ago

This PR introduces breaking API changes, it will form a 2.0 release of parfive.

fixes #38 fixes #29

This PR is motivated by the fact that we have been slowly growing more and more keyword arguments, environment variables etc at various levels in the class.

This PR cleans up the environment variable handling and keyword arguments to the main Downloader class by making a config object which holds the state for the instance. As well as this some of the more obscure keyword arguments are moved into a SessionConfig object.

Changes in this PR

codecov[bot] commented 2 years ago

Codecov Report

Merging #92 (106d5c2) into main (c52cbe3) will increase coverage by 0.84%. The diff coverage is 97.81%.

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   92.48%   93.33%   +0.84%     
==========================================
  Files           4        5       +1     
  Lines         466      540      +74     
==========================================
+ Hits          431      504      +73     
- Misses         35       36       +1     
Impacted Files Coverage Δ
parfive/downloader.py 93.26% <95.34%> (-0.41%) :arrow_down:
parfive/config.py 98.87% <98.87%> (ø)
parfive/utils.py 94.54% <100.00%> (+0.15%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c52cbe3...106d5c2. Read the comment docs.

Cadair commented 2 years ago

I am going to ping people who have contributed to see if anyone wants to comment on this PR, and give people a chance to ask for tweaks before I push a major release.

@GitHK @rlaker @nabobalis @dstansby

Cadair commented 2 years ago

Thanks for the review @GitHK, are you using the use_aiofiles= kwarg to Downloader? Would removing it cause you trouble?

GitHK commented 2 years ago

Thanks for the review @GitHK, are you using the use_aiofiles= kwarg to Downloader? Would removing it cause you trouble?

No issue for me. Go ahead and remove it. We usually do periodic updates and fix these small issues. It helps a lot when libraries point breaking changes in the changelog, upgrades are much smoother.

Cadair commented 2 years ago

@GitHK Thanks! I have pulled it now, I am going to keep headers around for people using previous versions of sunpy.

Cadair commented 2 years ago

I have pushed 2.0.0rc1 up to PyPI if people want to test this and open any issues.

nabobalis commented 2 years ago

Overall:

👍 on pydantic, such a great library. 👍 on the config refactor, makes things much clearer in the main downloader code. 👎 on adding type hints and using black.