Hi, I recently released my Flickr access library, which uses Twisted. I'd appreciate it if someone who knew the Twisted framework would have a look at the code and give it a quick review, to make sure I'm not doing anything stupid. The source is available via bzr: http://burtonini.com/bzr/flickrpc/ A tarball of 0.1 is also available: http://burtonini.com/computing/flickrpc-0.1.tar.gz Many thanks, Ross -- Ross Burton mail: ross@burtonini.com jabber: ross@burtonini.com www: http://www.burtonini.com./ PGP Fingerprint: 1A21 F5B0 D8D0 CFE3 81D4 E25A 2D09 E447 D0B4 33DF
Ross Burton wrote:
Hi,
I recently released my Flickr access library, which uses Twisted. I'd appreciate it if someone who knew the Twisted framework would have a look at the code and give it a quick review, to make sure I'm not doing anything stupid.
At a glance, it seems sane. One thing I noticed skimming: self.auth_getToken(frob=state['frob']).addCallbacks(gotToken, lambda fault: d.errback(fault)) could be just: self.auth_getToken(frob=state['frob']).addCallbacks(gotToken, d.errback) Actually, more importantly, this pattern is a bit odd:
def __getattr__(self, method, **kwargs): method = "flickr." + method.replace("_", ".") if not self.__methods.has_key(method): def proxy(method=method, **kwargs): d = defer.Deferred() def cb(data): self.logger.info("%s returned" % method) xml = ElementTree.XML(data) if xml.tag == "rsp" and xml.get("stat") == "ok": d.callback(xml) elif xml.tag == "rsp" and xml.get("stat") == "fail": err = xml.find("err") d.errback(Failure(FlickrError(err.get("code"), err.get("msg")))) else: # Fake an error in this case d.errback(Failure(FlickrError(0, "Invalid response"))) self.__call(method, kwargs).addCallbacks(cb, lambda fault: d.errback(fault)) return d self.__methods[method] = proxy return self.__methods[method]
Here the proxy function creates a deferred ("d"), effectively chains it to the Deferred from __call, then returns d. It would be simpler to just use the Deferred you already have. So something like (untested...): def __getattr__(self, method, **kwargs): method = "flickr." + method.replace("_", ".") if not self.__methods.has_key(method): def proxy(method=method, **kwargs): def cb(data): self.logger.info("%s returned" % method) xml = ElementTree.XML(data) if xml.tag == "rsp" and xml.get("stat") == "ok": d.callback(xml) elif xml.tag == "rsp" and xml.get("stat") == "fail": err = xml.find("err") raise FlickrError(err.get("code"), err.get("msg")) else: # Fake an error in this case raise FlickrError(0, "Invalid response") return self.__call(method, kwargs) self.__methods[method] = proxy return self.__methods[method] Which is noticeably simpler. You seem to do the same contortion everywhere. Also, rather than s.spawnlp(os.P_WAIT, "epiphany", "epiphany", "-p", url), perhaps the stdlib's "webbrowser" module would be more appropriate. Thanks for sharing this! -Andrew.
On Fri, 2007-01-19 at 14:35 +1100, Andrew Bennetts wrote:
At a glance, it seems sane. One thing I noticed skimming:
self.auth_getToken(frob=state['frob']).addCallbacks(gotToken, lambda fault: d.errback(fault))
could be just:
self.auth_getToken(frob=state['frob']).addCallbacks(gotToken, d.errback)
Ah yes, of course.
Actually, more importantly, this pattern is a bit odd:
def __getattr__(self, method, **kwargs): method = "flickr." + method.replace("_", ".") if not self.__methods.has_key(method): def proxy(method=method, **kwargs): d = defer.Deferred() def cb(data): self.logger.info("%s returned" % method) xml = ElementTree.XML(data) if xml.tag == "rsp" and xml.get("stat") == "ok": d.callback(xml) elif xml.tag == "rsp" and xml.get("stat") == "fail": err = xml.find("err") d.errback(Failure(FlickrError(err.get("code"), err.get("msg")))) else: # Fake an error in this case d.errback(Failure(FlickrError(0, "Invalid response"))) self.__call(method, kwargs).addCallbacks(cb, lambda fault: d.errback(fault)) return d self.__methods[method] = proxy return self.__methods[method]
Here the proxy function creates a deferred ("d"), effectively chains it to the Deferred from __call, then returns d. It would be simpler to just use the Deferred you already have. So something like (untested...):
def __getattr__(self, method, **kwargs): method = "flickr." + method.replace("_", ".") if not self.__methods.has_key(method): def proxy(method=method, **kwargs): def cb(data): self.logger.info("%s returned" % method) xml = ElementTree.XML(data) if xml.tag == "rsp" and xml.get("stat") == "ok": d.callback(xml) elif xml.tag == "rsp" and xml.get("stat") == "fail": err = xml.find("err") raise FlickrError(err.get("code"), err.get("msg")) else: # Fake an error in this case raise FlickrError(0, "Invalid response") return self.__call(method, kwargs) self.__methods[method] = proxy return self.__methods[method]
Which is noticeably simpler. You seem to do the same contortion everywhere.
Ah, I like the raising, I didn't know twisted would handle exceptions and pass them to the errback. Your re-written version doesn't actually call cb anywhere. I introduced a chained Deferred as I want to get the reply from the method call, and further process it before the user sees the reply. This is because a successful reply from the PoV of the HTTP request can actually contain an error message, which in cb() is converted to an exception. Also, the data from the HTTP call is processed and a different object is passed to the application's callback. I've only had my first coffee of the day so am obviously being a little slow, but where would the addCallback(cb) go in your version, and what would replace d.callback() inside cb()?
Also, rather than s.spawnlp(os.P_WAIT, "epiphany", "epiphany", "-p", url), perhaps the stdlib's "webbrowser" module would be more appropriate.
Yeah, good call. Thanks for the help, Ross -- Ross Burton mail: ross@burtonini.com jabber: ross@burtonini.com www: http://www.burtonini.com./ PGP Fingerprint: 1A21 F5B0 D8D0 CFE3 81D4 E25A 2D09 E447 D0B4 33DF
participants (2)
-
Andrew Bennetts
-
Ross Burton