eagleflo / mpyq

Python library for reading MPQ archives.
BSD 2-Clause "Simplified" License
99 stars 23 forks source link

Add version dependent inclusion of argparse as an install requirement. #13

Closed GraylinKim closed 12 years ago

GraylinKim commented 12 years ago

For versions prior to 2.7 argparse must be installed manually before mpyq can be installed for two reasons this patch addresses:

1. __version__ is imported from mpyq.py which requires all the
   dependencies to already be installed before the version can
   be imported.

2. The setup.py file doesn't include a install_requires keyword
   in the call to the setup functions.

By manually specifying the version in the setup.py file we avoid the chicken and the egg problem. By checking the current python version we can make sure to only require the argparse module for versions of python < 2.7.

GraylinKim commented 12 years ago

I also just realized that I only applied the force_decompress flag in one of the two spots block entry check points.

Added a commit for that as well.

I wanted to put it in a different pull request but Github doesn't seem to let you have more than one request open per repository.

eagleflo commented 12 years ago

I targeted Python 2.7+ purposefully when I first started the development of mpyq. The reason for this is twofold: I don't want to restrict myself to an older subset of Python and I don't want to have to test with multiple versions of Python. It has been a serious pain in the ass for me to support Python versions all the way down to 2.4 in the past in other projects.

That said, I like being pragmatic and if the library part otherwise works on Python 2.6 right now it could be worthwhile to support it as well.

Perhaps this problem could be avoided if I moved the import of argparse into the main function? That's the only place where it's used, and mpyq could still be used with Python 2.6 as a library. That way I also wouldn't have to remember to change the version from two places every time I make a new release. :)

I'll cherry-pick your second commit separately.

GraylinKim commented 12 years ago

Moving the import into the main sounds like a good workaround. I would still suggest the setup.py patch though so that people (even in 2.6) can pip install with all the dependencies like they might otherwise expect.

I completely understand on the 2.7 issue but unfortunately many (most?) linux distros still ship with Python 2.6.x and many people that haven't upgraded to the latest distro probably are also running 2.6.x. Also, I'm not sure how difficult it would be to get python2.7 running in the cygwin environment (for certain windows people). So there are a few reasons why it might be nice to support 2.6+.

Mpyq has worked fine in 2.6.X for me (and others I'm sure) in multiple environments in the past.

eagleflo commented 12 years ago

You speak the truth. I'll merge the first commit as well, with a slight change to avoid repeating the version number.

(For what it's worth, the two Linux distros I'm most familiar with, Ubuntu and Arch Linux, seem to ship with Python 2.7 and optionally Python 3.2 at this point.)

eagleflo commented 12 years ago

Merged in 51228ca43fdc27af9cf5c0a6b569e2b6a9d77cb7.