mrtolkien / leaguepedia_parser

A parser for the Leaguepedia website, focused on gathering esports data.
MIT License
71 stars 18 forks source link

Python 3.7 Support + Small Tweaks #1

Closed maxnumbers closed 4 years ago

maxnumbers commented 4 years ago

@mrtolkien Changes/Rationale below:

leaguepedia_parser/_tests/test.ipynb

leaguepedia_parser/parsers/game_parser.py

leaguepedia_parser/transmuters/game.py

leaguepedia_parser/transmuters/game_players.py and leaguepedia_parser/transmuters/tournament.py

mrtolkien commented 4 years ago

Thanks for the help! Unfortunately your pull request does not pass the tests and does not adhere to the coding guidelines of the project so I have to reject it at the moment.

Regarding python 3.7 support, I don’t think it is very wise to add it since almost all the type hints are TypedDicts. It will make every single file much more verbose without a real gain in the long run. I also sometimes use the walrus operator and the new f string options which cannot be ported to 3.7.

For the Jupyter notebook, I think it’s great for you to test stuff! But if you want to commit it to the repo it needs to be a standard pytest test file. We should not add random files to the repository.

Here are the steps to take for me to accept the PR:

Sorry for being such a pain and thanks again for the interest! GL HF.

maxnumbers commented 4 years ago

@mrtolkien It's completely understandable that you have an expected format! I've gotta take some time to understand pytest, as I had no idea that's what those modules were, and then I'll re-submit.

Thanks for taking the time to explain so thoroughly! :)