eagleflo / mpyq

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

Python3 Support #23

Closed GraylinKim closed 11 years ago

GraylinKim commented 11 years ago

Patches for arkx/mpyq#14.

I don't have sc2reader python3 compatible yet but I think this works. To test I ran mpyq -x on several different files in both python3 and python2 and verified that the extracted files were the same.

I would appreciate some additional QA work here before anything gets merged.

eagleflo commented 11 years ago

First of all, I appreciate your effort. :+1: Python 3 support has been a low hanging fruit for quite a while.

However, there are a couple of things which I'd like to improve before I'll pull this.

  1. These two commits could really be squashed into one. They make little sense individually, and the noise coming from print function change isn't really that bad.
  2. I'd rather not put the compatibility notes and comments in: Python 3 came out years ago and I feel like the differences and changes are an old hat for good Python developers. Let's trust them to know what to do and how to write compatible code without further guidance. I won't pull incompatible changes anyway. :)
  3. I think that there is no need for both StringIO and BytesIO. Unless I'm mistaken, the only other place where StringIO was being used could be converted to use BytesIO.
  4. We need to tell PyPI and others that this project is now Python 3 compatible in setup.py. Just need to add a few new classifiers there.

Anyway, thanks again for the pull request. If you'd like to do these fixes, go right ahead. Otherwise I'll work on this over the weekend. I'll also do some QA regardless.

GraylinKim commented 11 years ago

I'll fix this up tonight.

GraylinKim commented 11 years ago

Okay:

1) I squashed the commits. 2) The notes were in there for myself (first time looking at 2/3 compatibility) and you in case you needed them. I've stripped them out. 3) You are right, I'm actually surprised that the StringIO was writing the bytes without complaint, it maybe assumed the default platform encoding? 4) I added a general classifier for Python :: 3. Did you want to be more specific? I also changed the development status to Production/Stable since it seems like its definitely out of Alpha at this point. I can change it back if you want.

I also made the suggested code change you requested. I'm assuming you'll bump the version after you pull.

Let me know what else you find.

eagleflo commented 11 years ago

Almost there! Thank you for making the fixes.

There is some trouble with the changed formatting, though:

$ python3 --version
Python 3.3.0

$ python3 mpyq.py -Hb test.SC2Replay
MPQ archive hash table
----------------------
Hash A   Hash B  Locl Plat BlockIdx
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X
%08X %08X %04X %04X %08X

MPQ archive block table
-----------------------
Offset  ArchSize RealSize  Flags
Traceback (most recent call last):
File "mpyq.py", line 411, in <module>
main()
File "mpyq.py", line 403, in main
archive.print_block_table()
File "mpyq.py", line 297, in print_block_table
print('{0:0>8X} {1:0>8} {2:>8} {3:>8X}'.format(entry))
ValueError: Unknown format code 'X' for object of type 'str'
GraylinKim commented 11 years ago

I see. It looks like I forgot to convert one of the old format strings. It also seems that format doesn't automatically unpack named tuples.

Just pushed a fix in the commit.

eagleflo commented 11 years ago

Fixed in version 0.2.2. Thanks to Graylin Kim!