search

kelesi / mtga-utils

Magic the Gathering: Arena related stuff (Card collection export)
MIT License
53 stars 13 forks source link

Unix compatibility #13

Closed pak21 closed 4 years ago

pak21 commented 4 years ago

While I'm well aware MTGA is a Windows-only program, I still prefer to do my development work in a Unix-like environment, either Windows Subsystem for Linux or full-on dual-booted Linux. This PR is three small changes which makes that less painful:

  1. Set the executable bit on the top-level script and put the magic #! line at the top so Unix shells know it's a Python script.
  2. If APPDATA does exist and point at the right place, use os.path.join to build the path to the log file rather than explicit backslashes (which don't work on Unix)
  3. Don't blow up if APPDATA doesn't exist. The default value (assuming that the script is being run from the user's Windows home directory) probably isn't going to be very useful, but at least it now gets as far as showing an error message.
kelesi commented 4 years ago

The changes are reasonable, but I would prefer using #!/usr/bin/env python, because I try to make it compatible with both python 2 and 3.

pak21 commented 4 years ago

I don't fundamentally object to Python 2 support, but python-mtga seems to have dropped support? https://github.com/mtgatracker/python-mtga/blob/master/setup.py#L98 (and does break, self.lookup = {**abilities} on card_set.py:34 is invalid in Python 2). Do you have things working with Python 2?

kelesi commented 4 years ago

I maintain a fork of python-mtga compatible with python 2. It's only because pyInstaller did not work with Python 3. I can give it try again when the rotation happens. In the meantime please change the shebang line to python and I will merge the PR.

pak21 commented 4 years ago

I'll have to pull your fork and test as I found at least one bit in my code which is broken in Python 2 (the os.path.join(*foo, bar) call). Give me a couple of days... although it seems the latest MTGA update has broken the log file format which may be a bigger issue :-(

pak21 commented 4 years ago

OK, had good success actually playing Arena tonight so found some time to do this :-) I've tested with:

Both environments now successfully work with the test log file in tests. They fail with the log file actually being generated by yesterday's MTGA update, but I don't think that's the fault of this change.

kelesi commented 4 years ago

Thank you for the contribution!