search

nbatta / szar

10 stars 7 forks source link

Upgrade to Python3 and some documentation of test breakage #28

Closed dylancromer closed 6 years ago

dylancromer commented 6 years ago

I have run 2to3 on the code, fixing a few trivial breakages and one less trivial one in bin/makeGrids.py, where usage of z>pzcut and similar statements, with pzcut being defaulted to None, caused a TypeError due to differences in how Python 2 and 3 handle comparisons of None types.

The fix is via a hacky function defined at the top of the script which essentially replicates the Python2 behavior.

The .bak files are intact, containing the original Python 2 code.

msyriac commented 6 years ago

Could you remove the following files from the commit? They clutter up the tree and shouldn't be under version control. (a) All *.bak files -- there's no need for backups or originals when you're working with git version control (b) szar.egg.info* (c) szar/__pycache__* (d) *.png

Those last ones seem to have been added possibly through a "git add *" which you should avoid doing.

dylancromer commented 6 years ago

@msyriac Done.

Edit: Really done - missed some files. Commits should be squashed so as not to pollute the history.

dylancromer commented 6 years ago

Ah, I see. I was not aware we needed to maintain Python 2 support. 2to3 is the wrong tool for this. It only intends to fully convert 2 code to 3 code with no backwards compatibility; for this we should use something like python-future.

I made the changes to tests/ stuff when I was under the impression that these were more vitally important - I will avoid spending further time with them outside the above suggestions.

Probably the fastest thing to do is to roll back and then start from scratch with futurize; what I did with 2to3 didn't take long and was mostly just me playing around to learn the scripts. If I can't get quick results with futurize and minimal intervention, then perhaps you're right that this may be too much effort.

dylancromer commented 6 years ago

I have undone the 2to3 run, ran futurize, and fixed a few things manually. Futurize should have automatically added compatibility imports from future to all scripts needing them. This introduces a dependency on the future module.

I believe the fork should now reflect the changes you requested above.

Edit: @msyriac Tagging you in case you didn't see this. Also, just hit the wrong button, that's why this was closed for a second.