[Twisted-web] Twisted Flickr API library

[Andrew Bennetts]

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.