Blizzard / heroprotocol

Python library to decode Heroes of the Storm replays
MIT License
398 stars 69 forks source link

Converted the project into a proper python package. #5

Closed regner closed 4 years ago

regner commented 8 years ago

This should close out #4 if the pull request is accepted.

ghost commented 8 years ago

It's great work what you did but I really like it the way Blizzard intended it as a standalone tool, just as it is. It's much easier and more lightweight to use for the kind of project I'm doing for example and ports to another language are also easier to write since it's all there without any additional work.

Maybe it's better to have this here as a standalone tool and make a new repository that is a proper Python package, even if it's some extra maintenance then but it gives people more options.

riklaunim commented 8 years ago

Using packages doesn't exclude stand-alone version. It would be a ZIP built with setup.py that you could download.

regner commented 8 years ago

Curious how it ends up being easier to work with. You would now be able to pip install it for easier install and the command line API is still intact. It can of course still be downloaded as @riklaunim pointed out and installed locally. The only change there is it is an actual command line application so instead of running python heroprotocol.py [args] replay.StormReplay you just run heroprotocol [args] replay.StormReplay. You can also download the package same as before if you prefer that to pip installing.

As for porting to other languages this shouldn't really make a difference that much. The main work for that, if I understand the process right, is going to be in the decoders.py and the actual protocol*.py files. Restructuring the project like this has no effect on those files. Ports to other languages are going are going to want to structure their projects in ways that make sense for those languages as well.

This also opens up the ability to better solve #6 which makes this library a lot easier to work with for people developing applications in python.

ghost commented 8 years ago

Hm, you're right. Maybe I was just seeing my own port to Go and the integration into my project. The way it was without installing through setup.py was kind of straightforward but sure, it doesn't make much of a difference.

I'd have to download mpyq extra, though, which was the added work I didn't like about the whole thing and why I said I liked the standalone version as it was. But all in all, yes, of course, it's not a big deal and it's great that they released this in any form, especially under the MIT license.

regner commented 8 years ago

The nice thing about having it setup this way is when you run setup.py, or pip install, it will download mpyq and install it for you. :)

ghost commented 8 years ago

Just figured that out after I had written my comment. :+1: Haven't used Python for 5 years and only doing it now because of this. Thanks @Regner

regner commented 8 years ago

:D Happy to help! Looking forward to seeing a port of this in Go. That will be awesome.

ghost commented 8 years ago

Soon™. ;)

riklaunim commented 8 years ago

I hope it's not a dump-and-forget type of repository.

regner commented 8 years ago

Seems @tenarsis merged the open pull requests. Not sure if they plan on merging something this drastic but when i finds a few minutes I will merge the new commits into my branch so it can be merged. Here is hoping! :)

tenarsis commented 8 years ago

Hi Regner, like you said, your pull request is a pretty large change to the structure of the tool. We're not quite sure whether we would merge it in yet, but I really appreciate the effort that you've put into this. Thanks!

regner commented 8 years ago

Totally understand! :)

cherscarlett commented 8 years ago

Having it as a package is largely more pythonic than just the script using a package. I would consider having the script available as a download as well, though I can't possibly see how it is more useful than a package.

I used Regner's package branch to build a version that could be used without calling the script with args from shell. ^_^

On Sat, Jan 30, 2016 at 3:10 AM, Regner Blok-Andersen < notifications@github.com> wrote:

Totally understand! :)

— Reply to this email directly or view it on GitHub https://github.com/Blizzard/heroprotocol/pull/5#issuecomment-177149702.

Cher Stewart

You cannot do all the good the world needs, but the world needs all the good you can do.

riklaunim commented 8 years ago

+100 for non-shell usage too ;)

regner commented 8 years ago

If you're looking at using it not as a script and want a package check out this branch on my fork: https://github.com/regner/heroprotocol/tree/improve_api

It is based on my package work but moves everything into a nice object class that can be imported easily and worked with. I am still fiddling around with it but hope if this PR is accepted I can clean it up and submit it for #6.

cherscarlett commented 8 years ago

I already set up my own fork that loads the package as modules and am currently using it in another application. Thanks, though!

On Sat, Jan 30, 2016 at 11:21 AM, Regner Blok-Andersen < notifications@github.com> wrote:

If you're looking at using it not as a script and want a package check out this branch on my fork: https://github.com/regner/heroprotocol/tree/improve_api

It is based on my package work but moves everything into a nice object class that can be imported easily and worked with. I am still fiddling around with it but hope if this PR is accepted I can clean it up and submit it for #6 https://github.com/Blizzard/heroprotocol/issues/6.

— Reply to this email directly or view it on GitHub https://github.com/Blizzard/heroprotocol/pull/5#issuecomment-177280575.

Cher Stewart

You cannot do all the good the world needs, but the world needs all the good you can do.

regner commented 8 years ago

@tenarsis If you would like to see where I am hoping to go with this project see here: https://github.com/Regner/heroprotocol/tree/development

Essentially there are 3 pull requests in there so far (each is a separate branch on my fork right now). The first is this PR which starts by turning this all into a proper Python package. The second is improving the API and adds an actual python class HeroProtocol which implements all the actual methods. The third PR would be adding tests using pytest which runs each method from the HeroProtocol class against each known protocol. It doesn't confirm the output, simply that the stuff runs without crashing. A bit of a shame but adding example outputs as well would be... well a lot of work. This at least would give some confidence.

regner commented 8 years ago

I have added Travis (and Coveralls soon) to my fork so you can now see the tests.

One of the things that has come of this is the realization that mpyq has some kind of problem in python 3 which you can see here. I have removed those python 3 tests for now and opened an issue on the mpyq repo here. Not sure if anyone there is active still but will see.

Python 2.6 support is also lacking since importlib is not standard module in 2.6. A trimmed down version was added in 2.7. If support for 2.6 is desired that can be accomplished real easy by installing importlib.

regner commented 8 years ago

I am a complete idiot. That error I am getting has nothing to do with mpyq and is infact to do with the decoders and python 3. I think I need more sleep.

rcreasey commented 6 years ago

Closing due to entropy.