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] Change Files/ImagesPipelines class attributes to instance attributes #1891

Merged
merged 5 commits into from Apr 8, 2016

Conversation

@djunzu
Copy link
Contributor

@djunzu djunzu commented Mar 29, 2016

fix #1850
Addressed issues:

  1. Move default settings from FilesPipelines.py and ImagesPipelines.py to settings/default_settings.py
  2. Change class attributes from FilesPipelines.py and ImagesPipelines.py to instance attributes. It allows that distinct spiders running together have distinct custom settings for the pipelines.

Note: I have no way to test AWS related stuff. So I haven't touched any code related to AWS.

@kmike as you have looked at the issue before, I think you should review the PR.

@redapple
Copy link
Contributor

@redapple redapple commented Mar 30, 2016

@djunzu , would you mind rebasing against current "master" branch?
The Travis CI build failure for docs is now fixed.

@djunzu djunzu force-pushed the djunzu:update_files_images_pipelines branch from f90d071 to a9753dc Mar 30, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 30, 2016

Current coverage is 83.21%

Merging #1891 into master will increase coverage by +0.02% as of 7c59de3

Powered by Codecov. Updated on successful CI builds.

@djunzu
Copy link
Contributor Author

@djunzu djunzu commented Mar 30, 2016

Rebased.

I realized the changes were not backward compatible, so I rewrote it. Now it should be 100% backward compatible.

Note: If I am not wrong, some parts of the both pipelines were not covered by tests before. The only change I made in those parts was update the class attributes to a instance attribute. As there were no tests for those parts before (if I am not wrong), there are still no tests for it.


def __init__(self, store_uri, download_func=None):
def __init__(self, store_uri, settings=None, download_func=None):

This comment has been minimized.

@kmike

kmike Mar 30, 2016
Member

Could you please move settings argument to the end to make it backwards compatible for users who use positional arguments to create FilesPipeline instance?

This comment has been minimized.

@djunzu

djunzu Mar 30, 2016
Author Contributor

And I thought it was backwards compatible...
I will fix it.

@kmike
Copy link
Member

@kmike kmike commented Mar 30, 2016

@djunzu awesome, thanks for the PR!
Could you please add docs for new options?

@djunzu
Copy link
Contributor Author

@djunzu djunzu commented Mar 30, 2016

@kmike , there are no new options. They were all available before!
But indeed some of them are not documented. Is it enough to document them here?

@kmike
Copy link
Member

@kmike kmike commented Mar 30, 2016

@djunzu ouch, a good call! Some options are still not documented (see http://doc.scrapy.org/en/master/topics/settings.html#settings-documented-elsewhere - not all IMAGE/FILES options have links). But that's not directly related to this PR. If you can fix that it'd be great, but a PR looks good as-is 👍

djunzu added 4 commits Mar 31, 2016
	modified:   tests/test_pipeline_files.py
	modified:   tests/test_pipeline_images.py
	modified:   scrapy/pipelines/files.py
	modified:   scrapy/pipelines/images.py
	modified:   scrapy/settings/default_settings.py
	modified:   scrapy/pipelines/files.py
	modified:   tests/test_pipeline_files.py
@djunzu djunzu force-pushed the djunzu:update_files_images_pipelines branch from a9753dc to 2a12876 Mar 31, 2016
@djunzu
Copy link
Contributor Author

@djunzu djunzu commented Mar 31, 2016

If we are doing it, let's do it right!

@djunzu djunzu force-pushed the djunzu:update_files_images_pipelines branch 2 times, most recently from f1d598b to 3cabdbf Mar 31, 2016
	modified:   docs/topics/media-pipeline.rst
@djunzu djunzu force-pushed the djunzu:update_files_images_pipelines branch from 3cabdbf to 6988e9c Apr 2, 2016
@kmike kmike changed the title Change Files/ImagesPipelines class attributes to instance attributes [MRG+1] Change Files/ImagesPipelines class attributes to instance attributes Apr 8, 2016
@kmike
Copy link
Member

@kmike kmike commented Apr 8, 2016

Awesome, thanks @djunzu!

@redapple redapple merged commit 0ede017 into scrapy:master Apr 8, 2016
1 of 2 checks passed
1 of 2 checks passed
codecov/patch 89.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
redapple added a commit that referenced this pull request Apr 12, 2016
[backport][1.1] Change Files/ImagesPipelines class attributes to instance attributes (PR #1891)
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.

4 participants
You can’t perform that action at this time.