search

iamgreaser / iceball

Open-source rewrite of the VOXLAP version of Ace of Spades.
http://iceball.build
GNU General Public License v3.0
113 stars 32 forks source link

refactor heartbeat.py to simplify protocol changes #199

Closed NotAFile closed 8 years ago

NotAFile commented 9 years ago

It's not beautiful, but at least in a state I would want to work on it.

This also needs to switch to BaseHTTPServer for less not-invented-here, will open issue.

NotAFile commented 9 years ago

it automatically gets garbage collected once it leaves scope

NotAFile commented 9 years ago

indeed, my thoughts were that would help if changed away from http

NotAFile commented 9 years ago

I'll get on this tomorrow

rakiru commented 9 years ago

To make those comments, you go to the code view.

p is in global scope, thus never goes out of scope. Look into how Python scoping works. I added that for a reason.

NotAFile commented 9 years ago

huh? not for me. Both when running the actual code (omitted for length) and the example below, p goes out of scope, and gets garbage collected

EDIT: Apparently it only drops out of scope when I run the little snippet p is created and defined. Not sure whats going on. I can't get p to be global even if I try. nevermind, I did something horrendously stupid when doing my original test if the del is needed. Embarassing.

many other things seem to be a personal style matter, so I'll adjust them to the way you like to do things

rakiru commented 9 years ago

Python only really has global scope and function scope. try/except, for, with, etc. don't create a new scope.

It's not really making it my personal style as just not changing it to your style. Things like the isinstance check were perfectly fine already. The None in (foo, bar) thing is arguably less clear, but also less efficient (it has to create a tuple each time then iterate over it, rather than do two hardcoded comparisons), and again is you changing it to your own style.

Edit: For the line comments, you go to Files changed, hover over a line and click the blue + that appears. Existing comments have an Add a line note button under them.

NotAFile commented 9 years ago

Yes, I agree. Is there anything more I can do to get this merged? I do know about line notes.

rakiru commented 9 years ago

If the things I've commented on have been sorted, then it should be good to go. I'll leave this for @iamgreaser to merge, since it's him that runs the server anyway.

NotAFile commented 9 years ago

no Interest? If so feel free to close!

iamgreaser commented 9 years ago

I thought you said to hold off the merge for a bit because there was a bug.

rakiru commented 9 years ago

Yeah, that was the last thing I saw on this.

NotAFile commented 9 years ago

Already fixed. I'm afraid I wasn't as clear as I should have been. Should have written "Fixes the blocker bug" or something along those lines.