kanghyojun commented 5 years ago

Modified setup.py to use install_requires correctly.
Used tox to run on a various version of Python.
Added compatibility function for ord. As I mentioned in here, It needs to support both Python2 and Python3.
Replace % syntax to str.format().
Replace xrange() into range().
Replace .iteritems() into items().
Used print function.
Create s2_cli.py command

jrepp commented 5 years ago

I really like this change except for one aspect: Could you remove the dependency on the 'six' module? It's one less thing to install and it's only used to determine if we're running Python 3 or not.

kanghyojun commented 5 years ago

@jrepp I've just modified it(https://github.com/Blizzard/s2protocol/pull/86/commits/ab233d64a942d526dec6af1aec80f7a80adabf22)! But I have one concern that this PR would make a bug. Is there a more test suite?

jrepp commented 5 years ago

Missing tests is has definitely been a weakness of this library.

I just added a new test and travis configuration to master that runs python 2.7 and python 3.6 with a handful of replays from sc2replaystats.com

Go ahead and rebase your PR and verify it against https://travis-ci.org/Blizzard/s2protocol

I'd love to see more tests get added as well especially with a commit of this size.

Do you have any ideas?

kanghyojun commented 5 years ago

@jrepp I rebased my PR from the master. I don't think I have a good idea for testing, But file-based testing that you've just added is a good start.

I found s2protocol dosen't support Python3 when I try to decode SC2Reply on my project. And there is one bug on this code See https://travis-ci.org/Blizzard/s2protocol/jobs/489398369

kanghyojun commented 5 years ago

And I fixed it. (Added Python3.7 support as well :))

kanghyojun commented 5 years ago

According to my understanding of @jrepp 's comment (https://github.com/Blizzard/s2protocol/pull/87#issuecomment-461132606), It is necessary to test s2_cli as well.

And I've added 3 commits to test it properly. (https://github.com/Blizzard/s2protocol/pull/87#issuecomment-468547349 )

~So Can I close this PR though I've got approved from @StanczakDominik ?~ I've just brought some changes from https://github.com/Blizzard/s2protocol/pull/87

kanghyojun commented 5 years ago

@jrepp I'd appreciate it if you would review this PR. :)

kanghyojun commented 5 years ago

@AntyMew Fixed it! I don't know why my riggrep dosen't catch the .iteritems() calling.

AntyMew commented 5 years ago

I thought that would fix the problems with game events, message events, and tracker events in s2_cli.py, but apparently no such luck. map returns a generator instead of a list in python 3, effectively making map(func, iterable) a no-op as a standalone statement

https://github.com/kanghyojun/s2protocol/blob/cd670a8d7573453e76f4d6f72252a497159d2888/s2protocol/s2_cli.py#L354-L368

jrepp commented 5 years ago

Sorry this is taking me so long to review. I actually am running into local issues with the new module imports specifically in the command line module.

Traceback (most recent call last): File "s2protocol\s2_cli.py", line 15, in from .versions import build, list_all, latest ValueError: Attempted relative import in non-package

I went back to read PEP328 and verify my assumptions around testing this code. There are some work arounds that are not PEP328 compatible in master. This is not to say that master is correct but that my local testing process is not currently working with the changes.

kanghyojun commented 5 years ago

@jrepp I think the issue occured because s2_cli.py is not a script that included in package so i created entry point to run s2_cli.py.

So If you don't like console script the way i did, use absolute import to fix that issue.

kanghyojun commented 5 years ago

@AntyMew Got it. I should consider to use list comprehension or for statement instead.

jrepp commented 5 years ago

So If you don't like console script the way i did, use absolute import to fix that issue.

How are you testing the console script?

If there is a use case change I want to understand it and document it properly. This is the primary way that people use the library current afaik.

jrepp commented 5 years ago

I started testing using python -m s2protocol.s2_cli locally which works great for installed packages and developing against relative imports in the directory. I updated s2_cli.py with a relative import for attributes and the documentation in a local branch of your work here:

https://github.com/jrepp/s2protocol/tree/kanghyojun-py3-compatibility

Feel free to cherry-pick those commits and respond to the other good feedback contained in the other replies.

This is looking good though. I think with some additional cleanup we can get this into master.

kanghyojun commented 5 years ago

@jrepp I mean that console script is this. (It creates s2_cli.py when install s2protocol package from PyPI) So I test this command in tox like this.

How do you feel about this? I prefer the above way over python -m module solution.

$ s2_cli.py --all tests/s2replaystatsdata/2018-11-29_Z_iaguz_VS_P_Luneth.SC2Replay

jrepp commented 5 years ago

How do you feel about this? I prefer the above way over python -m module solution.

I prefer this as well but it doesn't work unless you've installed the package whereas the python -m syntax works for both installed and downloaded stand-alone. I can include a note about this in the readme.

jrepp commented 5 years ago

Additionally, there still exists some mentions of iteritems: line 118 of s2_cli.py, and in _varuint32_value in versions/protocol* files

I found this reference, I'm not sure if option 2 or 3 is preferabble. Maybe one of you with more python 3 experience can chime in. The code generator that is maintained from internal sources will need to be updated and re-run for all known versions to re-build the protocolXXXXX.py files

https://python-future.org/compatible_idioms.html

Iterable dict items:

Python 2 only:

for (key, value) in heights.iteritems(): ...

Python 2 and 3: option 1

for (key, value) in heights.items(): # inefficient on Py2 ...

Python 2 and 3: option 2

from future.utils import viewitems

for (key, value) in viewitems(heights): # also behaves like a set ...

Python 2 and 3: option 3

from future.utils import iteritems

or

from six import iteritems

for (key, value) in iteritems(heights): ...

kanghyojun commented 5 years ago

@jrepp I understood what you said! I hope to merge this PR soon! Thanks for your comments.

jrepp commented 5 years ago

@kanghyojun recently I got a lot closer

Keep in mind I no longer actually work on SC2 so my time is very limited :)

I was able to resurrect the internal code generation system which I had re-written a few years ago for the deep mind integration. I'm now able to re-generate protocols again back to version 1.0.0.15405 (wings of liberty gold master I believe)

Currently working through how to make the generated modules work for both python 2 and 3. I'm thinking of maintaining two parallel directories until we fully remove python 2 support.

kanghyojun commented 5 years ago

Oh! I didn't know that you hadn't worked on SC2 anymore. Thanks for your time!👍

And Is there any way to help you to fix code generator? If the code generator is open source as well, i willing to make a pull request.

If you want to make a two directory for two python version, You can use package_dir in setuptools.setup.

jrepp commented 5 years ago

@kanghyojun I just updated master with updated protocol files that will work with both Python 2 & 3!

We should be able to merge master and have a much improved version of s2protocol ready to go!

On open sourcing internal tools and methods: Unfortunately we can't release some internal data structures and tools due to our anti-cheating/hacking efforts.

kanghyojun commented 5 years ago

@jrepp Awesome! Thanks for your attention and works.

Blizzard / s2protocol

Support Python3 compatibility #86

Python 2 only:

Python 2 and 3: option 1

Python 2 and 3: option 2

Python 2 and 3: option 3

or