Fixes toJson in sc2reader/scripts/sc2json.py #118

rohits47 · 2020-07-06T03:44:23Z

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!


        fix toDict in replay plugin

rohits47 · 2020-07-06T03:47:27Z

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:

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
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 · 2020-07-07T05:19:48Z

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 · 2020-07-07T23:29:02Z

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 · 2020-07-08T05:23:45Z

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

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

sc2reader/scripts/sc2json.py

cclauss · 2020-07-08T05:32:30Z

The typo is fixed in #119


        Update sc2reader/scripts/sc2json.py

Fixed type - Committed suggestion from PR review #118 Co-authored-by: Christian Clauss <cclauss@me.com>

rohits47 · 2020-07-08T05:42:49Z

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!

fix toDict in replay plugin

Loading status checks…

6a32c33

rohits47 changed the base branch from master to upstream Jul 6, 2020

StoicLoofah reviewed Jul 6, 2020

View changes

cclauss suggested changes Jul 8, 2020

View changes

sc2reader/scripts/sc2json.py Outdated Show resolved Hide resolved

cclauss approved these changes Jul 8, 2020

View changes

StoicLoofah approved these changes Jul 9, 2020

View changes

ggtracker / sc2reader

Fixes toJson in sc2reader/scripts/sc2json.py #118

Fixes toJson in sc2reader/scripts/sc2json.py #118

rohits47 commented Jul 6, 2020

rohits47 commented Jul 6, 2020

StoicLoofah left a comment

cclauss commented Jul 7, 2020 •

edited

StoicLoofah commented Jul 7, 2020

cclauss commented Jul 8, 2020

cclauss commented Jul 8, 2020

rohits47 commented Jul 8, 2020

cclauss left a comment

StoicLoofah left a comment

ggtracker / sc2reader

Join GitHub today

Fixes toJson in sc2reader/scripts/sc2json.py #118

Fixes toJson in sc2reader/scripts/sc2json.py #118

Conversation

rohits47 commented Jul 6, 2020

rohits47 commented Jul 6, 2020

StoicLoofah left a comment

cclauss commented Jul 7, 2020 • edited

StoicLoofah commented Jul 7, 2020

cclauss commented Jul 8, 2020

cclauss commented Jul 8, 2020

rohits47 commented Jul 8, 2020

cclauss left a comment

StoicLoofah left a comment

cclauss commented Jul 7, 2020 •

edited