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] Allow redirections in media files downloads #2616

Merged
merged 17 commits into from Mar 16, 2017

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Mar 3, 2017

Continuation of #2193
Fixes #2004 and #2449

Compared to #2193, I chose to remove MEDIA_HTTPSTATUS_LIST: I don't think this level of detail on status codes is needed for redirections. Debatable.

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 3, 2017

Codecov Report

Merging #2616 into master will increase coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2616      +/-   ##
=========================================
+ Coverage   84.14%   84.3%   +0.15%     
=========================================
  Files         161     162       +1     
  Lines        9063    9122      +59     
  Branches     1344    1353       +9     
=========================================
+ Hits         7626    7690      +64     
+ Misses       1177    1170       -7     
- Partials      260     262       +2
Impacted Files Coverage Δ
scrapy/pipelines/media.py 97.22% <100%> (+9.17%)
scrapy/pipelines/files.py 69.91% <100%> (+4.23%)
scrapy/core/downloader/handlers/s3.py 62.9% <0%> (-32.26%)
scrapy/utils/boto.py 46.66% <0%> (-26.67%)
scrapy/core/downloader/tls.py 80% <0%> (-8.58%)
scrapy/spiders/sitemap.py 56.66% <0%> (-1.4%)
scrapy/core/downloader/handlers/http11.py 89.64% <0%> (-1.2%)
scrapy/core/engine.py 92.3% <0%> (-0.07%)
scrapy/commands/startproject.py 100% <0%> (ø)
scrapy/commands/version.py 91.11% <0%> (ø)
... and 13 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 7e8453c...871134e. Read the comment docs.

@redapple redapple changed the title [WIP] Allow redirections in media files downloads [MRG] Allow redirections in media files downloads Mar 3, 2017
@redapple redapple added this to the v1.5 milestone Mar 3, 2017
@@ -249,3 +257,61 @@ def test_use_media_to_download_result(self):
self.assertEqual(new_item['results'], [(True, 'ITSME')])
self.assertEqual(self.pipe._mockcalled, \
['get_media_requests', 'media_to_download', 'item_completed'])


class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase):

This comment has been minimized.

@kmike

kmike Mar 9, 2017
Member

How hard would it be to have an integration-style test for this?

This comment has been minimized.

@redapple

redapple Mar 10, 2017
Author Contributor

I'll have a go at it.

This comment has been minimized.

@redapple

redapple Mar 10, 2017
Author Contributor

@kmike , I added some resources to MockServer to list files and redirect, and used similar tests to tests/test_crawl.py

@kmike kmike modified the milestones: v1.4, v1.5 Mar 10, 2017
@kmike kmike merged commit ec55799 into scrapy:master Mar 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike
Copy link
Member

@kmike kmike commented Mar 16, 2017

Looks good, thanks @redapple!

@kmike
Copy link
Member

@kmike kmike commented Mar 16, 2017

@redapple how does it fix #2449?

@redapple
Copy link
Contributor Author

@redapple redapple commented Mar 16, 2017

@kmike , looks like I misread #2449. It has nothing to do with redirects.

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Jun 15, 2017

@kmike @redapple what do you think of adding MEDIA_ALLOW_REDIRECTS = True to the new project template, so that it's on by default for new projects?

@redapple
Copy link
Contributor Author

@redapple redapple commented Jun 15, 2017

@lopuhin , sounds fine to me.

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Jun 15, 2017

@redapple thanks, I'll make a PR!

@lopuhin
Copy link
Member

@lopuhin lopuhin commented Jun 15, 2017

Ah, although I see that media pipeline is not enabled in the sample project settings, so it might seem strange to have a setting that does nothing... And it's already documented, so someone who will be enabling this pipeline will see this setting too.

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