alexis-mignon / python-flickr-api

A python implementation of the Flickr API
BSD 3-Clause "New" or "Revised" License
367 stars 108 forks source link

Uses Pillow for Image processing #56

Closed eloyz closed 9 years ago

eloyz commented 9 years ago

flickr_api.objects.Photo.show was raising an Exception.

In [2]: photo.show('Square')
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-2-273cd7b12160> in <module>()
----> 1 photo.show('Square')

/Users/eloy/Code/python-flickr-api/flickr_api/objects.py in show(self, size_label)
   1339             size_label = self._getLargestSizeLabel()
   1340         r = urllib2.urlopen(self.getPhotoFile(size_label))
-> 1341         b = cStringIO.StringIO(r.read())
   1342         Image.open(b).show()
   1343 

NameError: global name 'cStringIO' is not defined

I'm using Python 2.7.8 on a Macbook Air OS 10.10.2 Pillow has been good to me.

Thanks for the project! Loads of help deleting photo sets by name. Helped a lot. Ended up making a shell that I used while command-line batch editing photos. If you're interested I'd love to share as Pull-Request. Warning it's dependent on iPython.

alexis-mignon commented 9 years ago

Hello,

Thanks for your interest in flickr_api. It is not clear for me, what the problem was since it still works correctly for me. I suppose that it comes from pillow which does not expose Image as a first level module as pillow does.

Could you please confirm that ? If it is the case, then the code modification should work with both PIL and pillow making the depencie on pillow unnecessary.

Moreover bracketing the import with a try/catch that does nothing was probably an error from my side.

Alexis

alexis-mignon commented 9 years ago

Hello again,

Reading back the code, I realized that the reason why the import of Image was bracketed by a try/catch statement was because I found out that, while convenient, the feature allowing to show an image was not a key feature of the package. As such I do not want the package to be unusable because non core feature is unavailable, hence the try/catch.

However, it is clearly a bad practice to let the the catch block empty, this leading to obscure errors while a clear message should appear. I would recommand, along with the "from PIL import Image" correction to add a dummy definition of "Image" in the "catch" block. Something like what follows.

import warnings
from cStringIO import StringIO

try:
    from PIL import Image    
except ImportError:
    def Image(*args, **kwargs):
        warnings.warn(
              "The PIL package was not found on this system. Images cannot be displayed.\n"
              "Consider installing PIL or Pillow."
        )
        raise RuntimeError("Image module not found.")

What do you think ?

Alexis

eloyz commented 9 years ago

Perfect!

Noticed this line from cStringIO import StringIO ... I guess we'd just have to make sure to update line b = cStringIO.StringIO(r.read()),

Thank you for the quick response! Appreciate everything!

eloyz commented 9 years ago

Made the update locally but ended up getting the following error instead of what I expected.

AttributeError                            Traceback (most recent call last)
<ipython-input-2-273cd7b12160> in <module>()
----> 1 photo.show('Square')

/Users/eloy/Code/python-flickr-api/flickr_api/objects.py in show(self, size_label)
   1358         r = urllib2.urlopen(self.getPhotoFile(size_label))
   1359         b = StringIO(r.read())
-> 1360         Image.open(b).show()
   1361 
   1362     @static_caller("flickr.photos.getUntagged")

AttributeError: type object 'Image' has no attribute 'open'

The error does make sense. How do you feel about the following change?

try:
    from PIL import Image    
except ImportError:

    class Image(object):

        @classmethod
        def open(cls, *args, **kwargs):
            warnings.warn(
                  "The PIL package was not found on this system. Images cannot be displayed.\n"
                  "Consider installing PIL or Pillow."
            )
            raise RuntimeError("Image module not found.")
alexis-mignon commented 9 years ago

Since there is no need to access the class 'cls' itself the open method should be a 'staticmethod' instead of a 'classmethod' imho. Except for that it looks good to me.

eloyz commented 9 years ago

Updated

alexis-mignon commented 9 years ago

Thanks for the fix