mikeboers / Flask-Images

On-demand resizing of images for Flask applications.
https://mikeboers.github.io/Flask-Images/
BSD 3-Clause "New" or "Revised" License
81 stars 43 forks source link

Use setuptools and Pillow #10

Closed iurisilvio closed 10 years ago

iurisilvio commented 10 years ago

I did it to be able to run python setup.py test and python setup.py develop.

I replaced Pillow with PIL, because PIL is not setuptools compatible and is almost abandoned. Pillow is a drop-in replacement.

iurisilvio commented 10 years ago

@mikeboers Any thoughts about this pull request?

I know I'm changing your dependencies, maybe you still depends on PIL, but it is almost dead. I'm using Pillow because PIL just doesn't work with setuptools and I wanted to use python setup.py test to run tests.

http://pillow.readthedocs.org/en/latest/about.html

mikeboers commented 10 years ago

I have only a basic understanding of this PIL/pillow madness, so take my opinions with a grain of salt.

I think the only sensible thing to do is to not list either of them as dependencies. This tool is not the place to decide for the user which one they must use.

Aside, I think it is really unfortunate that whoever decided that a fork was necessary thought that they should take over the namespace as well. This seems like it will result in the same clusterfuck that FFmpeg and Libav have found themselves in, with no real benefit to anyone.

So for this lowly Flask extension, perhaps simply rewriting the ImportError output a reasonable description if the user doesn't have either installed is the real direction to go. Maybe.

iurisilvio commented 10 years ago

I agree with you, but I didn't found a declarative way to check if PIL or Pillow is installed. It is possible to dynamically define install_requires:

install_requires = ['Flask']
try:
    import PIL
except ImportError:
    install_requires.append('Pillow')

I found people doing this:

Maybe this is the right thing to do.

I don't know what happens if I have install_requires = ['Flask-Images', 'PIL']. Probably this is documented somewhere.

mikeboers commented 10 years ago

In my quick tests it seems that whichever package is the last to install, wins. As such, I don't think it should be in the requirements at all, and only mentioned if you don't have it at runtime.

Sucks, but seems the most responsible.

mikeboers commented 10 years ago

Sorry to keep sitting on this. I spent some time (unfortunately) harassing the Pillow author on Twitter, and want to put some more thought into it.

I think I'll end up pulling what you gave me...

mikeboers commented 10 years ago

I've ended up making https://github.com/mikeboers/pillowcase, which could serve as the dependency (and prefer pillow)

mikeboers commented 10 years ago

Aaaaand merged, and released as 1.1.2.

Thanks for your (extreme) patience!

iurisilvio commented 10 years ago

Awesome! Thanks!