ValvePython / dota2

🐸 Python package for interacting with Dota 2 Game Coordinator
http://dota2.readthedocs.io
204 stars 32 forks source link

Implementation of parties #7

Closed m-bo-one closed 7 years ago

m-bo-one commented 8 years ago

Code for quick test https://gist.github.com/DeV1doR/b23da9657a9a019f3629fc45dc1ecf3a

Issue https://github.com/ValvePython/dota2/issues/5

rossengeorgiev commented 8 years ago

Hi @DeV1doR, thanks for the PR.

I see a lot of changes related to style, like indents and line breaking. I'm happy to discuss such changes, but they are only adding noise to this PR. Could you move them to a separate PR? I assume that you are using something like pylint or flake8 where it screams for lines longer than 79 characters. My advice is to up the limit to 100 or 120, which is a more forgiving limit. We certainly have the screen-space for it. I suspect the other such changes such as default function/method parameters are due to such warnings.

I see you've put URLs to protobuf messages in the docstrings. Unfortunately, when a proto file changes, those lines won't match. I generally avoided adding them in the docs for that reason. However you can get a URL in its' canonical form, which will reflect the file state at that commit.

  1. Go to https://github.com/ValvePython/dota2/blob/master/protobufs/base_gcmessages.proto#L118
  2. Press y
  3. Page is reloaded to https://github.com/ValvePython/dota2/blob/ca75440adca20d852b9aec3917e4387466848d5b/protobufs/base_gcmessages.proto#L118
m-bo-one commented 8 years ago

So I need to return all indents, line breaks and make new pull request right?

rossengeorgiev commented 8 years ago

You just make further commits that revert those changes. client.py you can revert quickly with:

git checkout ca75440 dota2/client.py

m-bo-one commented 8 years ago

I returned back this changes. New commit as https://github.com/ValvePython/dota2/pull/7/commits/500214b068c2075c6b7d4ac7327128e23b88c3cf

m-bo-one commented 8 years ago

TODO (After SO):

add auto remove dota2.party property, when user decline party;

I think this part not need be, because SO will do this. I will remove it from PR description.

m-bo-one commented 7 years ago

Merged in v0.2.3.