[Twisted-web] Re: Twisted Flickr API library

[Ross Burton]

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 at burtonini.com
                                          jabber: ross at burtonini.com
                                     www: http://www.burtonini.com./
 PGP Fingerprint: 1A21 F5B0 D8D0 CFE3 81D4 E25A 2D09 E447 D0B4 33DF