New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #210 and some refactoring #214

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@xNinjaKittyx
Copy link
Contributor

xNinjaKittyx commented Jan 10, 2019

Fixes Bug #210 by not crashing, and spits out a message "Not a utf-8 string"

Took the liberty to generally refactor protocols.py in two ways.

  1. I noticed one for loop that used the for key, value in dict.items():, which I then applied to all for loops where applicable.

  2. dict['key'] if key in dict else None pattern has been changed to dict.get('key') and if None is something else like 0 then you will see dict.get('key', 0)

Edit: Would also like to mention that this should improve performance slightly since it'll only do a lookup in the dict once instead of twice.

I also changed handleMessages to use a dict instead of a series of if/elif/else. It takes a more EAFP approach and makes it a little easier to read in my opinion. Let me know if you like or dislike the change.

Tested it working in a Python 3.6 environment (server + 2 clients)

xNinjaKittyx added some commits Jan 10, 2019

Merge pull request #3 from Syncplay/master
Update with Master
Fixes #210, Refactors the dict[key] if key in dict pattern to dict.ge…
…t(), handleMessages if/elif statement uses a dictionary, json.loads(line) is no longer with a bare except
@Et0h

This comment has been minimized.

Copy link
Contributor

Et0h commented Jan 16, 2019

I would need to do extensive testing before allowing a refactor because the last time I allowed one it broke things in subtle ways that were hard to detect and track down. I do not currently have time for this at present so I will leave this PR open for now.

Et0h added a commit that referenced this pull request Feb 1, 2019

@Et0h

This comment has been minimized.

Copy link
Contributor

Et0h commented Feb 1, 2019

Hopefully resolved now by fb9b3ce.

@Et0h Et0h closed this Feb 1, 2019

albertosottile added a commit to albertosottile/syncplay that referenced this pull request Feb 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment