Code review comment for lp:~wyuka/zeitgeist-datasources/purple

Revision history for this message
Michal Hruby (mhr3) wrote :

First of all, I really don't think logging every sent and received url is a good idea, I'd get rid of all of that. (anyway if the user clicks on the url, it'll be logged by the browser dataproviders). @manish: objections?

Second, signed-on/off are not too interesting either imo, at very least they shouldn't be USER_ACTIVITY.

Let's get to something more specific (line numbers as seen in LP's diff):
681 727: libzg has method (zeitgeist_interpretation_for_mimetype) which turns mime-type into Interpretation, use that please.
...,686,...: storage parameter for ZeitgeistSubject is often wrong, local files are never "net".

Other than that there are a couple of leaks:
619: account_uri
671, 717: file
672, 718: parent

I'm no purple plugin expert, but I'd say you should disconnect the signals in the unload method.

review: Needs Fixing (code)

« Back to merge proposal