Code review comment for lp:~hazmat/txzookeeper/managed-watch-and-ephemeral

Revision history for this message
Jim Baker (jimbaker) wrote :

+1, LGTM, just some minors

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py
File txzookeeper/managed.py (right):

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode153
txzookeeper/managed.py:153: def cb_restablish_session(self, e=None):
Presumably this should be cb_re_establish... but I don't see the
difference here with the easier synonym: restore. Good either way with
me.

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode162
txzookeeper/managed.py:162: # If its been explicitly closed, don't
restablish.
it's... re-establish

(or are you saying something about being restful? ;) )

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode166
txzookeeper/managed.py:166: # If its a stale handle, don't restablish
it's... re-establish

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode174
txzookeeper/managed.py:174: # Its already been restablished, don't
restablish.
It's... re-established... re-establish

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode183
txzookeeper/managed.py:183: # Restablish
I'm not going to mark any more of these!

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newcode189
txzookeeper/managed.py:189: log.error("error while restablish %r %s" %
(e, e))
Another minor: probably want some consistency in beginning with
uppercase. Not certain why you use log.error here, but log.exception
later for what appears to be a similar case.

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/tests/test_managed.py
File txzookeeper/tests/test_managed.py (right):

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/tests/test_managed.py#newcode110
txzookeeper/tests/test_managed.py:110: """Reset fires a synthentic
client event, and clears watches.
no need for comma

https://codereview.appspot.com/5976074/

« Back to merge proposal