FilesPipeline extracted and separated from ImagesPipeline

[loucash]

• ImagesPipeline interface has not changes
• ImagesPipeline has been built on top of FilesPipeline
• tests for FilesPipeline and ImagesPipeline has been separated
• new tests for FilesPipeline: file_key generation and file expiration testing

FilesPipeline functionality is:

• file downloading
• minimize network transfers and file processing, doing stat of the files and determining if file is new, uptodate or expired.

ImagesPipeline functionality is:

• converting images to JPG
• image thumbnail generation logic

[dangra]

@redapple and @whodatninja worked on the same problem in #250, what do you think guys?

I like this patch keeps compatibility for ImagesPipeline API and add tests for FilesPipelines.

#250 also adds some extra settings and functionality to ImagesPipeline that are better covered by an extra patch.

[redapple]

could be called stat_file

[loucash]

done

[redapple]

Small type: Overridable

[loucash]

yup, fixed

[redapple]

Not really sure if my comment should be part of this change, but there was a question the other day on StackOverflow about changing the filename (for ImagesPipeline) based not only on URL, but also based on some info from the originating item. One of my ideas was to additionally pass the request or response to image_key(). It's helpful in case you add meta to Request in get_media_requests()
My answer to http://stackoverflow.com/questions/18081997/scrapy-customize-image-pipeline-with-renaming-defualt-image-name/18083143 suggests something else though

[loucash]

Well, I would like to treat those changes rather as a refactorization that extracts FilesPipeline and keep the interface.

I think new functionalities is a separated topic for a separated pull request, probably built on this one.

[redapple]

Sure.

[redapple]

I think you need to keep the call to image_key for people having overriden it

[loucash]

yup, fixed within last commit

[redapple]

here also, I would keep self.pipeline.image_key

[loucash]

Yes, I thought about it, and maybe it is better to keep file_key. Let me explain:
ImagesPipeline inherits from FilesPipeline and FilesPipeline internals requires file_key.

If someone, by accident removes "def file_key" from ImagesPipeline then image_key will never be used and results will be wrong.

So, here, by using file_key, you test also a pointer from file_key to image_key in ImagesPipeline.

[redapple]

I see.

[dangra]

everyone involved satisfied with this patch?

[redapple]

Looks ok to me.

[loucash]

Looks good to me.

[max-arnold]

Has anyone considered changing file_key(), image_key() and thumb_key() methods to accept Request instead of url? This will allow to pass some context from an item by overriding get_media_requests() and using Request.meta.

To do that right now it is necessary to override a lot of methods (especially media_to_download(), media_downloaded() and file_downloaded() in files.py which call file_key() and are quite monolithic):

http://stackoverflow.com/questions/12956653/scrapy-create-folder-structure-out-of-downloaded-images-based-on-the-url-from
http://stackoverflow.com/questions/18081997/scrapy-customize-image-pipeline-with-renaming-defualt-image-name

[redapple]

@max-arnold I had commented in this thread some time ago around those ideas
#370 (diff)

A few ideas I'd like to see were in #250

[dangra]

This issue pop ups from time to time, and I think it was never addressed because nobody took the task og implementing it in a backward compatible way.

We considered it in the past but only with the Request object because in media_to_download hook the Response is not available.

+1 to merge something like this if it has tests and it's backwards compatible.

[max-arnold]

Is something like this looks fine in terms of backward compatibility (same with image_key and thumb_key)?

    def file_key(self, request):
        if isinstance(request, Request):
            url = request.url
        else:
            url = request
            log.warning('Passing url to file_key() is deprecated, please use Request instance instead')

        media_guid = hashlib.sha1(url).hexdigest()
        media_ext = os.path.splitext(url)[1]
        return 'full/%s%s' % (media_guid, media_ext)

[dangra]

@max-arnold : no, that isn't enough because subclasses defining its own file_key() expects an url and won't handle a Request.

we need a new method to wrap file_key() and others.

[dangra]

I'm feeling bad for naming the new methods, what do you think about this names:

• file_key -> file_path
• image_key -> file_path # note: same name than file_key is on purpose
• thumbs_key -> thumbnail_path

[dangra]

the new methods signature will be:

def file_path(request, response=None, info=None):

[max-arnold]

Ok, I need some time to think and prepare a pull request.

[dangra]

thx

[dangra]

@redapple: join the party! what do you think about the new methods and its signatures? are you OK with this approach too?

[max-arnold]

See #490