search

eagleflo / mpyq

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

Several small fixes for python3 compatibility. #28

Closed GraylinKim closed 10 years ago

GraylinKim commented 10 years ago

Discovered these additional python3 issues while using the command line options.

eagleflo commented 10 years ago

Hey, thanks for the pull request.

In general, I prefer commits which do only one thing at a time. Some people call these kind of commits "atomic". Here fixing iteritems is clearly one atomic operation, and changing the empty file write from "" to b'' is another.

Would you mind going into further detail about that second change, changing "" to b''? It isn't clear to me at a glance as to what it's going to change or fix.

eagleflo commented 10 years ago

Just to add a little detail: by separating the atomic commits from each other, I can pull these changes separately. I could in theory also decline to pull a commit or more likely ask for further refinements for a single commit. In case something goes wrong I can rollback a single atomic commit without worrying about what else will break.

Since in this case it's such a trivial operation, I'll fix the iteritems issue myself.

eagleflo commented 10 years ago

Just one additional thought: we really do need some rudimentary tests for all supported switches to really make sure that mpyq keeps working on both Python 2 and Python 3 in the future. This was broken for a long time.

I'll roll up my sleeves.

GraylinKim commented 10 years ago

Okay, that's fine. I guess I got a little bit lazy with my commit. As described in the new commit. The b'' indicates that the empty string is a bytes object (instead of a string object) in python3 and is ignored in python2.

You can only write byte strings to files opened in binary mode in python3.

eagleflo commented 10 years ago

Thanks for your work, released 0.2.5 with these fixes.

(I need to add an archive with empty files to the test suite.)