Closed flying-sheep closed 10 years ago
flying-sheep
commented
10 years ago I really appreciate it.
cool!
There are some details which I would like to ask to you fix them
i’m pretty drunk (2:30am here), so it took like half an hour to fix this, but since the tests still pass, i think we’re fine.
ralhei
commented
10 years ago Hi Philipp, sorry for the late response ... I've looked at your last commit - is there a reason the new OOBCallback class is contained in rconn.py, and not in test_rparser.py where it is actually is used? You you have any broader usage of this class in mind, one which would be useful on the API level? I just curious about this because I would like to keep the API rather small and clean if possible.
One thing about the naming of this class: Since it temporarily changes the callback method, would be a name like TmpOOBCallback be a bit more descriptive on what it does?
flying-sheep
commented
10 years ago hi, i was on vacation, thus the late reply.
about the placement: i thought it would be useful that it’s available in the API, as i think this tends to be the way you use callbacks in more complex applications, no? if you have simple use cases, you set-and-forget the callback; if you have more complex ones, you use that context manager to set one, use it inside of the with block, and have a connection object without callback afterwards.
furthermore, __exit__ is always called, so if there is an exception, the callback is unset anyway, if you use the context manager.
i did some research, and indeed, context managers seem to be either functions that modify an object and are named as inflected verb (like closing), or classes with fitting names like -Stack, -Block or -Lock. TmpOOBCallback seems to be a good idea, but also simply calling.
Alternatively, we make it a method of the connection, like:
conn = rconn.connect()
with conn.calling(my_callback):
conn.r('self.oobMessage(2, code=10L)')If we do that, we could make the callback itself “private” with a leading underscore, and only teach about this method in the docs.
What do you think?
PS: in any case, i should add the context manager to the documentation prose ☺
ralhei
commented
10 years ago Hi Philipp, I've merged in your code - thanks for contributing and discussing it. I've added a bit more of documentation around OOB to show a somehow practical application of this feature. The context manager is not yet documented, but will come soon. Will bump the version number of pyRserve to 0.8.0 quite soon as well. R
As discussed per mail, I tried to add OOB capabilities without breaking either the lib or readability.
I think I’ve succeeded.
PS: really good code, a pleasure to work with. Finding the places to extend was surprisingly easy, and in fact, implementing oobSend without oobMessage or callbacks ran on the first try!
The only thing I didn’t test was running it in DEBUG mode, sorry. This might create the problem I talk about in the line comment, but no problem in getting a new version out ;)