Code review comment for lp:~hazmat/txzookeeper/session-event-handling

Excerpts from Jim Baker's message of Tue Jun 21 00:18:33 UTC 2011:
> Review: Approve
> +1, we certainly need this branch, and it looks quite solid.
>
> In addition to Gustavo's comments:
>
> [1]
>
Added most of the grammatical changes.
>
> [2]
>
> + http://bit.ly/mQrOMY
> + http://bit.ly/irKpfn
>
> Why not just expand these links out? Alternatively use a readable URL
> (the underlying nabble URL is modestly readable, I don't know how
> permalink either one is, however).
>

The links are rather ugly by themselves and violate 80 col lines.

>
> [3]
>
> + assert callable(callback)
>
> Shouldn't this be throwing a TypeError instead? Also, there is no
> correponding assertion (or verification) on
> set_connection_error_callback. Lastly, there is no test on this path.

Well the tests definitely exercised it by way of using the functionality of the callback, but i've gone ahead and added the typeerror and invalid callback tests.

>
>
> [4]
>
> + def xtest_session_expired_event(self):
>
> Do you want to include this test?
>

nice catch, i did but it was before the implementation of the session work, i've moved the test to test_session

thanks for the review,

cheers,

Kapil