Fixes toJson in sc2reader/scripts/sc2json.py

[rohits47]

Fixes toJson in sc2reader/scripts/sc2json.py to not take encoding - encoding is not a kwarg in json.dumps in python3.7 . I'm not sure how to add tests for cli/argparse code and there don't seem to be existing tests for that part, so I hope that's ok? Also not sure about the guidelines for python2/python3 support, but this was run with python3.7.8 which should be the current main stable supported version AFAIK. I tried this patch out locally and it seemed to work for me:

before:

(starcraft-toolkit) rohit@macbook-pro ➜  starcraft2toolkit git:(master) ✗ python sc2reader/sc2reader/scripts/sc2json.py my_sample_replays/sample_replay_TvP.SC2Replay
Traceback (most recent call last):
  File "sc2reader/sc2reader/scripts/sc2json.py", line 45, in <module>
    main()
  File "sc2reader/sc2reader/scripts/sc2json.py", line 40, in main
    replay_json = factory.load_replay(args.path[0])
  File "/Users/rohit/.pyvenvs/starcraft-toolkit/lib/python3.7/site-packages/sc2reader-1.5.0-py3.7.egg/sc2reader/factories/sc2factory.py", line 85, in load_replay
  File "/Users/rohit/.pyvenvs/starcraft-toolkit/lib/python3.7/site-packages/sc2reader-1.5.0-py3.7.egg/sc2reader/factories/sc2factory.py", line 143, in load
  File "/Users/rohit/.pyvenvs/starcraft-toolkit/lib/python3.7/site-packages/sc2reader-1.5.0-py3.7.egg/sc2reader/factories/sc2factory.py", line 154, in _load
  File "/Users/rohit/.pyvenvs/starcraft-toolkit/lib/python3.7/site-packages/sc2reader-1.5.0-py3.7.egg/sc2reader/factories/plugins/utils.py", line 20, in call
    ...         2. is still persisted
  File "/Users/rohit/.pyvenvs/starcraft-toolkit/lib/python3.7/site-packages/sc2reader-1.5.0-py3.7.egg/sc2reader/factories/plugins/replay.py", line 22, in toJSON
  File "/usr/local/opt/python/Frameworks/Python.framework/Versions/3.7/lib/python3.7/json/__init__.py", line 238, in dumps
    **kw).encode(obj)
TypeError: __init__() got an unexpected keyword argument 'encoding'

after:

(starcraft-toolkit) rohit@macbook-pro ➜  starcraft2toolkit git:(master) python sc2reader/sc2reader/scripts/sc2json.py my_sample_replays/sample_replay_TvP.SC2Replay
{"region": "us", "map_name": "Port Aleksander LE", "file_time": null, "filehash": "33d1afc2fbc66302163786b490cae0c336c862e104f5ce1f4df2856f33b156f5", "unix_timestamp": 1555084416, "date": "2019-04-12 15:53:36", "utc_date": null, "speed": "Faster", "category": "Ladder", "type": "1v1", "is_ladder": true, "is_private": false, "filename": "my_sample_replays/sample_replay_TvP.SC2Replay", "frames": 14306, "build": 73286, "release": "4.8.4.73286", "game_fps": 16.0, "game_length": 638, "players": [{"avg_apm": null, "color": {"r": 180, "g": 20, "b": 30, "a": 255, "name": "Red"}, "handicap": 100, "name": "Floyd", "pick_race": "Protoss", "pid": 1, "play_race": "Protoss", "result": "Win", "type": null, "uid": 0, "url": "http://us.battle.net/sc2/en/profile/649084/1/Floyd/", "messages": []}, {"avg_apm": null, "color": {"r": 0, "g": 66, "b": 255, "a": 255, "name": "Blue"}, "handicap": 100, "name": "grEEngLade", "pick_race": "Terran", "pid": 2, "play_race": "Terran", "result": "Loss", "type": null, "uid": 1, "url": "http://us.battle.net/sc2/en/profile/7594852/1/grEEngLade/", "messages": []}], "observers": [], "real_length": 638, "real_type": "1v1", "time_zone": -7.0, "versions": [1, 4, 8, 4, 73286, 73286]}

Please let me know if there's a better way I should be doing this, or if there are any other issues with the PR. Thanks!

[rohits47]

Note: the failing style check in circleci doesn't seem to be from my change 🤔

https://app.circleci.com/pipelines/github/ggtracker/sc2reader/1/workflows/62349467-aaf9-4cf9-9c23-d07a27a2ce65/jobs/471/steps

WARNING: Binary file: ./test_s2gs/summary.s2gs 
 
./docs/source/articles/conceptsinsc2reader.rst:58: augument ==> argument, augment
 
 
Exited with code exit status 1 
CircleCI received exit code 1 

[StoicLoofah]

Thanks for submitting this! I have a few suggestions in order of preference:

1. Ideally, we make the code both Python2 and Python3 compatible. If Python3 doesn't have an equivalent parameter, then we should drop it in that case. If it's running in Python2, then we should pass it
2. Alternatively, we could just drop it altogether. In that case, I would recommend that yu also remove the encoding as an argument to this function since it does nothing.

I'm not too concerned about test coverage here. I would prefer it, but I can understand if you don't want to put the extra work in to cover something that already isn't covered.

And yes, the spellcheck appears to be unrelated to your code. Since you hit on it, can you fix the spelling on that as well?

[cclauss]

try:
    factory.register_plugin(
         "Replay", toJSON(encoding=args.encoding, indent=args.indent)
     )  # legacy Python
except TypeError:
     factory.register_plugin("Replay", toJSON(indent=args.indent))

However, #85 (comment) Python 2 died 188 days ago on 1/1/2020.
I see no reason for us to continue to support legacy Python.

[StoicLoofah]

True, but I don't see why we should drop support if it is convenient enough, and in this case, it appears to be so.

On this issue, if the encoding parameter is still supported in python3 in another way, we should make it work. If it isn't used, then we should remove the command line arg entirely.

[cclauss]

I don't see why we should drop support [for legacy Python]

1. We are encouraging our users to rely on an insecure platform
2. We are increasingly our testing complexity and number of code paths
3. We are unable to use modern capabilities such asyncio, f-strings, and type annotations

[cclauss]

The typo is fixed in #119

[rohits47]

Sorry for the late reply only catching up on this conversation now - to clarify though:

• my commit was ran/tested on python 3, so I assume the encoding argument is python-2-only (and should therefore be deprecated/removed) - I'll verify this and update here accordingly
• I didn't test this at all on python 2 - I agree it should be fairly easy to support both parameter structures (i.e. pass encoding for python 2 and not pass it at all for python 3) - I can take a stab at that and add that to this PR

@cclauss fixed the typo (thanks!) - I added that as a commit to this PR

[cclauss]

LGTM

[StoicLoofah]

Thanks for the change!