Code review comment for lp:~dholbach/harvest/packagesets

=== added file 'harvest/common/launchpad.py'
--- harvest/common/launchpad.py 1970-01-01 00:00:00 +0000
+++ harvest/common/launchpad.py 2010-01-22 17:07:20 +0000
@@ -0,0 +1,38 @@
+from launchpadlib.launchpad import Launchpad, EDGE_SERVICE_ROOT
+from launchpadlib.credentials import Credentials
+from launchpadlib.errors import HTTPError
+
+from django.conf import settings
+
+import sys
+import os
+
+def lp_login(lp_instance=EDGE_SERVICE_ROOT):

A docstring here would be really helpful. Something like:

    """Return a logged in launchpad object."""

+ cachedir = os.path.join(settings.PROJECT_PATH, 'lp_data/cache')
+ creddir = os.path.join(settings.PROJECT_PATH, 'lp_data/lp_credentials')
+ project = settings.PROJECT_NAME.strip()
+ if not os.path.isdir(creddir):
+ os.makedirs(creddir)
+ cred = os.path.join(creddir, '%s.credentials' % project)
+
+ if os.path.exists(cred):
+ credentials = Credentials()
+ credentials.load(open(cred))
+ launchpad = Launchpad(credentials, lp_instance, cachedir)
+ else:
+ try:
+ launchpad = Launchpad.get_token_and_login(project, lp_instance,
+ cachedir)
+ except HTTPError, e:
+ print >> sys.stderr, 'Error connecting to Launchpad: %s' % str(e)
+ sys.exit(1)

It would be better to raise an exception here, instead of printing to standard error and exiting. Sometimes the e part of the except is rather unhelpful.

+ f = open(cred, 'w')
+ os.chmod(cred, 0600)
+ launchpad.credentials.save(f)
+ f.close()
+ return launchpad
+
+def get_packagesets(lp):
+ current_series = lp.distributions['ubuntu'].current_series
+ packagesets = filter(lambda a: a.distroseries == current_series, lp.packagesets)
+ return packagesets

=== modified file 'harvest/opportunities/models.py'
--- harvest/opportunities/models.py 2010-01-22 17:07:20 +0000
+++ harvest/opportunities/models.py 2010-01-22 17:07:20 +0000
@@ -7,9 +7,12 @@
 TYPE_GREEN_THRESHOLD = 200
 TYPE_RED_THRESHOLD = 1000

+class PackageSet(models.Model):
+ name = models.SlugField(_("Name"), max_length=40)

 class SourcePackage(models.Model):
     name = models.SlugField(_("Name"), max_length=70)
+ packagesets = models.ManyToManyField(PackageSet, null=True)

     class Meta:
         ordering = ['name']

So, by this model definition, is it safe to say that packages can be in many different package sets? Have you tested this at all? I think we need a property on packagesets that can link back to the SourcePackages as well.