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

Allow passing of custom arguments to urllib Request object #34

Merged
merged 9 commits into from
Jul 26, 2019

Conversation

DashLt
Copy link
Contributor

@DashLt DashLt commented Jul 21, 2019

This will allow for more granular control of headers, proxies, and anything else that urllib provides an option for without having to implement the functionality in pySmartDL. Closes #33

@codecov
Copy link

codecov bot commented Jul 21, 2019

Codecov Report

Merging #34 into master will decrease coverage by 1.49%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #34     +/-   ##
=========================================
- Coverage   72.54%   71.05%   -1.5%     
=========================================
  Files           5        5             
  Lines         641      646      +5     
=========================================
- Hits          465      459      -6     
- Misses        176      187     +11
Impacted Files Coverage Δ
pySmartDL/pySmartDL.py 69.72% <58.33%> (-0.54%) ⬇️
pySmartDL/download.py 71.21% <75%> (+0.62%) ⬆️
pySmartDL/utils.py 69.29% <0%> (-6.3%) ⬇️

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 ff24208...e4b67ff. Read the comment docs.

Copy link
Owner

@iTaybb iTaybb left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. This is generally great and thank you for your time.
I have few minor cosmetic changed I would like you to change before merging in.
Please let me know if you have the time to fix it.

Thanks

@@ -75,14 +77,21 @@ class SmartDL:
* If no path is provided, `%TEMP%/pySmartDL/` will be used.
'''

def __init__(self, urls, dest=None, progress_bar=True, fix_urls=True, threads=5, timeout=5, logger=None, connect_default_logger=False):
def __init__(self, urls, dest=None, progress_bar=True, fix_urls=True, threads=5, timeout=5, logger=None, connect_default_logger=False, requestArgs=None):
Copy link
Owner

@iTaybb iTaybb Jul 21, 2019

Choose a reason for hiding this comment

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

  1. this is user-facing API and therefore requestArgs should become request_args
  2. I feel like it should come after "fix_urls" or something, and not at the end of the constructor.
  3. The place must be in sync with the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 and 3 have been fixed. As for 2, I put it at the end as to not break any code that currrently uses positional arguments. I can change it if you'd like though.

@iTaybb
Copy link
Owner

iTaybb commented Jul 26, 2019

Thanks

@iTaybb iTaybb merged commit 925fc53 into iTaybb:master Jul 26, 2019
iTaybb added a commit that referenced this pull request Jul 26, 2019
  - fixes over MR #34
  - get_json method for SmartDL
  - better logging syntax
  - added tests
  - change test files host from hetzner.de to ovh
iTaybb added a commit that referenced this pull request Jul 26, 2019
  - fixes over MR #34
  - get_json method for SmartDL
  - better logging syntax
  - added tests
  - change test files host from hetzner.de to ovh
iTaybb added a commit that referenced this pull request Jul 26, 2019
  - fixes over MR #34
  - get_json method for SmartDL
  - better logging syntax
  - added tests
  - change test files host from hetzner.de to ovh
iTaybb added a commit that referenced this pull request Jul 26, 2019
  - fixes over MR #34
  - get_json method for SmartDL
  - better logging syntax
  - added tests
  - updated user agents
  - change test files host from hetzner.de to ovh
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.

Add support for passing custom urllib.request.Request object
2 participants