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

[MRG+1] data URI download handler. #2334

Merged
merged 7 commits into from Mar 6, 2017
Merged

[MRG+1] data URI download handler. #2334

merged 7 commits into from Mar 6, 2017

Conversation

@ArturGaspar
Copy link
Contributor

@ArturGaspar ArturGaspar commented Oct 18, 2016

GitHub does not let me change head branch in #2175, so I have to open a new PR.

Fixes #2156

@codecov-io
Copy link

@codecov-io codecov-io commented Oct 18, 2016

Codecov Report

Merging #2334 into master will increase coverage by 0.04%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2334      +/-   ##
==========================================
+ Coverage    83.9%   83.95%   +0.04%     
==========================================
  Files         161      162       +1     
  Lines        8893     8935      +42     
  Branches     1312     1317       +5     
==========================================
+ Hits         7462     7501      +39     
  Misses       1176     1176              
- Partials      255      258       +3
Impacted Files Coverage Δ
scrapy/settings/default_settings.py 98.62% <ø> (ø)
scrapy/core/downloader/handlers/datauri.py 93.33% <93.33%> (ø)
scrapy/core/downloader/handlers/http11.py 90.83% <0%> (-0.31%)
scrapy/utils/misc.py 91.66% <0%> (ø)
scrapy/core/downloader/handlers/ftp.py 98.21% <0%> (+0.03%)
scrapy/utils/python.py 85.62% <0%> (+0.17%)
scrapy/downloadermiddlewares/stats.py 92.3% <0%> (+0.3%)
scrapy/utils/response.py 85.41% <0%> (+0.31%)
... and 1 more

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 ce8a9aa...b50d037. Read the comment docs.

@redapple
Copy link
Contributor

@redapple redapple commented Feb 8, 2017

@ArturGaspar , w3lib 1.17 will be released soon (see scrapy/w3lib#87)

@redapple
Copy link
Contributor

@redapple redapple commented Feb 8, 2017

@ArturGaspar , I relaunched the Travis builds with w3lib 1.17. All green :)

@dangra
Copy link
Member

@dangra dangra commented Feb 8, 2017

FileDownloadHandler class is in a module named file, HTTPDownloadHanlder is in a module named http so I would name the file datauri.py instead of data.py, it is analogous to class name DataUriDownloadHanlder.

@dangra
Copy link
Member

@dangra dangra commented Feb 8, 2017

This PR should bump required minimum w3lib version to 1.17 in scrapy's setup.py

@kmike kmike mentioned this pull request Feb 13, 2017
@kmike
Copy link
Member

@kmike kmike commented Feb 21, 2017

Could you please rebase it on master?

There was a few other places w3lib version should be bumped, and a few other pull requests which needed this bump, so the required w3lib version is now increased in master.

@kmike kmike added this to the v1.4 milestone Feb 21, 2017
@ArturGaspar ArturGaspar force-pushed the ArturGaspar:data_uri branch from 1c7ab1c to 3a6a9d5 Feb 22, 2017
@ArturGaspar ArturGaspar force-pushed the ArturGaspar:data_uri branch from 3a6a9d5 to 3139f4a Feb 22, 2017
@redapple
Copy link
Contributor

@redapple redapple commented Feb 24, 2017

Looks good to me.
@dangra , @kmike , what do you think?

@kmike
Copy link
Member

@kmike kmike commented Feb 27, 2017

@ArturGaspar looks good!

Could you please add a test (or modify an existing test) to check other Response attributes: url, headers?

@kmike kmike changed the title [WIP] data URI download handler. [MRG+1] data URI download handler. Mar 2, 2017
@kmike
Copy link
Member

@kmike kmike commented Mar 2, 2017

Looks good, thanks @ArturGaspar!

@redapple redapple merged commit 0a17f9b into scrapy:master Mar 6, 2017
3 checks passed
3 checks passed
codecov/patch 93.33% of diff hit (target 83.9%)
Details
codecov/project 83.95% (+0.04%) compared to ce8a9aa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants