From 539d34bce08c6c0bf19f8ae31e9d77271c3e22b7 Mon Sep 17 00:00:00 2001 From: Pawel Miech Date: Wed, 15 Jun 2016 15:39:11 +0200 Subject: [PATCH] [media-pipeline, file-pipeline] allow setting custom settings for subclasses * move key_for_pipe function to media pipeline so that file pipeline can use it * use key_for_pipe in file pipeline so that users can define custom settings for subclasses easily * add tests for file pipelines attributes and settings --- scrapy/pipelines/files.py | 15 ++++++-- scrapy/pipelines/images.py | 40 ++++++++++---------- scrapy/pipelines/media.py | 16 ++++++++ tests/test_pipeline_files.py | 73 +++++++++++++++++++++++++++++++----- 4 files changed, 109 insertions(+), 35 deletions(-) diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index 3e6ad554d..b9c43dc3b 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -229,11 +229,18 @@ class FilesPipeline(MediaPipeline): if isinstance(settings, dict) or settings is None: settings = Settings(settings) - + + cls_name = "FilesPipeline" self.store = self._get_store(store_uri) - self.expires = settings.getint('FILES_EXPIRES', self.EXPIRES) - self.files_urls_field = settings.get('FILES_URLS_FIELD', self.DEFAULT_FILES_URLS_FIELD) - self.files_result_field = settings.get('FILES_RESULT_FIELD', self.DEFAULT_FILES_RESULT_FIELD) + self.expires = settings.getint( + self._key_for_pipe('FILES_EXPIRES', cls_name), self.EXPIRES + ) + self.files_urls_field = settings.get( + self._key_for_pipe('FILES_URLS_FIELD', cls_name), self.DEFAULT_FILES_URLS_FIELD + ) + self.files_result_field = settings.get( + self._key_for_pipe('FILES_RESULT_FIELD', cls_name), self.DEFAULT_FILES_RESULT_FIELD + ) super(FilesPipeline, self).__init__(download_func=download_func) diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index 465d7c492..de616211e 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -53,27 +53,25 @@ class ImagesPipeline(FilesPipeline): if isinstance(settings, dict) or settings is None: settings = Settings(settings) - def key_for_pipe(key): - """ - Allow setting settings for user defined ImagePipelines that inherit from base. - - User can define setting key: - - MYPIPELINENAME_IMAGE_SETTING_NAME = - - and it will override default settings and class attributes. - """ - class_name = self.__class__.__name__ - if class_name == "ImagesPipeline": - return key - return "{}_{}".format(class_name.upper(), key) - - self.expires = settings.getint(key_for_pipe('IMAGES_EXPIRES'), self.EXPIRES) - self.images_urls_field = settings.get(key_for_pipe('IMAGES_URLS_FIELD'), self.IMAGES_URLS_FIELD) - self.images_result_field = settings.get(key_for_pipe('IMAGES_RESULT_FIELD'), self.IMAGES_RESULT_FIELD) - self.min_width = settings.getint(key_for_pipe('IMAGES_MIN_WIDTH'), self.MIN_WIDTH) - self.min_height = settings.getint(key_for_pipe('IMAGES_MIN_HEIGHT'), self.MIN_HEIGHT) - self.thumbs = settings.get(key_for_pipe('IMAGES_THUMBS'), self.THUMBS) + cls_name = "ImagesPipeline" + self.expires = settings.getint( + self._key_for_pipe('IMAGES_EXPIRES', cls_name), self.EXPIRES + ) + self.images_urls_field = settings.get( + self._key_for_pipe('IMAGES_URLS_FIELD', cls_name), self.IMAGES_URLS_FIELD + ) + self.images_result_field = settings.get( + self._key_for_pipe('IMAGES_RESULT_FIELD', cls_name), self.IMAGES_RESULT_FIELD + ) + self.min_width = settings.getint( + self._key_for_pipe('IMAGES_MIN_WIDTH', cls_name), self.MIN_WIDTH + ) + self.min_height = settings.getint( + self._key_for_pipe('IMAGES_MIN_HEIGHT', cls_name), self.MIN_HEIGHT + ) + self.thumbs = settings.get( + self._key_for_pipe('IMAGES_THUMBS', cls_name), self.THUMBS + ) @classmethod def from_settings(cls, settings): diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 21b8b8986..740312f8f 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -27,6 +27,22 @@ class MediaPipeline(object): def __init__(self, download_func=None): self.download_func = download_func + + def _key_for_pipe(self, key, base_class_name): + """ + Allow setting settings for user defined MediaPipelines that inherit from base. + + User can define setting key: + + MYPIPELINENAME_IMAGE_SETTING_NAME = + + and it will override default settings and class attributes. + """ + class_name = self.__class__.__name__ + if class_name == base_class_name: + return key + return "{}_{}".format(class_name.upper(), key) + @classmethod def from_crawler(cls, crawler): try: diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index 4c64f6f3e..fd54b7229 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -191,9 +191,9 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): "DEFAULT_FILES_RESULT_FIELD": "files" } file_cls_attr_settings_map = { - ("EXPIRES", "FILES_EXPIRES"), - ("DEFAULT_FILES_URLS_FIELD", "FILES_URLS_FIELD"), - ("DEFAULT_FILES_RESULT_FIELD", "FILES_RESULT_FIELD") + ("EXPIRES", "FILES_EXPIRES", "expires"), + ("DEFAULT_FILES_URLS_FIELD", "FILES_URLS_FIELD", "files_urls_field"), + ("DEFAULT_FILES_RESULT_FIELD", "FILES_RESULT_FIELD", "files_result_field") } def setUp(self): @@ -216,7 +216,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): if not prefix: return settings - return {prefix.upper() + "_" + k: v for k, v in settings.items()} + return {prefix.upper() + "_" + k if k != "FILES_STORE" else k: v for k, v in settings.items()} def _generate_fake_pipeline(self): @@ -235,12 +235,11 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): custom_settings = self._generate_fake_settings() another_pipeline = FilesPipeline.from_settings(Settings(custom_settings)) one_pipeline = FilesPipeline(self.tempdir) - for pipe_attr, settings_attr in self.file_cls_attr_settings_map: + for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: default_value = self.default_cls_settings[pipe_attr] self.assertEqual(getattr(one_pipeline, pipe_attr), default_value) custom_value = custom_settings[settings_attr] - pipe_attr_lower = pipe_attr.lower().replace("default_", "") - self.assertEqual(getattr(another_pipeline, pipe_attr_lower), custom_value) + self.assertEqual(getattr(another_pipeline, pipe_ins_attr), custom_value) def test_subclass_attributes_preserved_if_no_settings(self): """ @@ -248,9 +247,63 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): """ pipe_cls = self._generate_fake_pipeline() pipe = pipe_cls.from_settings(Settings({"FILES_STORE": self.tempdir})) - for pipe_attr, settings_attr in self.file_cls_attr_settings_map: - attr_lower = pipe_attr.lower().replace("default_", "") - self.assertEqual(getattr(pipe, attr_lower), getattr(pipe, pipe_attr)) + for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: + self.assertEqual(getattr(pipe, pipe_ins_attr), getattr(pipe, pipe_attr)) + + def test_subclass_attrs_preserved_custom_settings(self): + """ + If file settings are defined but they are not defined for subclass class attributes + should be preserved. + """ + pipeline_cls = self._generate_fake_pipeline() + settings = self._generate_fake_settings() + pipeline = pipeline_cls.from_settings(Settings(settings)) + for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: + value = getattr(pipeline, pipe_ins_attr) + self.assertEqual(value, getattr(pipeline, pipe_attr)) + + def test_no_custom_settings_for_subclasses(self): + """ + If there are no settings for subclass and no subclass attributes, pipeline should use + attributes of base class. + """ + class UserDefinedFilesPipeline(FilesPipeline): + pass + + user_pipeline = UserDefinedFilesPipeline.from_settings(Settings({"FILES_STORE": self.tempdir})) + for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: + # Values from settings for custom pipeline should be set on pipeline instance. + custom_value = self.default_cls_settings.get(pipe_attr.upper()) + self.assertEqual(getattr(user_pipeline, pipe_ins_attr), custom_value) + + def test_custom_settings_for_subclasses(self): + """ + If there are custom settings for subclass and NO class attributes, pipeline should use custom + settings. + """ + class UserDefinedFilesPipeline(FilesPipeline): + pass + + prefix = UserDefinedFilesPipeline.__name__.upper() + settings = self._generate_fake_settings(prefix=prefix) + user_pipeline = UserDefinedFilesPipeline.from_settings(Settings(settings)) + for pipe_attr, settings_attr, pipe_inst_attr in self.file_cls_attr_settings_map: + # Values from settings for custom pipeline should be set on pipeline instance. + custom_value = settings.get(prefix + "_" + settings_attr) + self.assertEqual(getattr(user_pipeline, pipe_inst_attr), custom_value) + + def test_custom_settings_and_class_attrs_for_subclasses(self): + """ + If there are custom settings for subclass AND class attributes + setting keys are preferred and override attributes. + """ + pipeline_cls = self._generate_fake_pipeline() + prefix = pipeline_cls.__name__.upper() + settings = self._generate_fake_settings(prefix=prefix) + user_pipeline = pipeline_cls.from_settings(Settings(settings)) + for pipe_cls_attr, settings_attr, pipe_inst_attr in self.file_cls_attr_settings_map: + custom_value = settings.get(prefix + "_" + settings_attr) + self.assertEqual(getattr(user_pipeline, pipe_inst_attr), custom_value) class TestS3FilesStore(unittest.TestCase):