Launchpad itself

Code review comment for lp://staging/~jtv/launchpad/db-bug-766807

db-bug-766807
Merge into db-devel

Revision history for this message

William Grant (wgrant) wrote on 2011-04-27:

Looks mostly good, but a few comments:

43 +[create_distroseries_indexes]
44 +# The database role that these jobs will run in.
45 +# datatype: string
46 +dbuser: archivepublisher

Can you change JobCronScript to take the dbuser directly, rather than continuing the proliferation of pointless config sections?

79 + :param distroseries: A newly created `DistroSeries` that needs
80 + its archive indexes created.

The series may have been created some time ago; it was merely recently initialised.

135 +def get_addresses_for(person):
136 + """Get list of contact email address(es) for `person`."""

How does this differ from lp.registry.model.person.get_recipients?

145 +class CreateDistroSeriesIndexesJob(DistributionJobDerived):
146 + """Job to create a distroseries's archive indexes.
147 +
148 + To do this it runs publish-distro on the distribution, in careful mode.
149 + """

I'd prefer clarification that it operates only on the primary and partner archives, and only in careful *indices* mode.

190 + return "initializing archive indexes for %s" % self.distroseries.title

DistroSeries.title is pretty much useless... even __str__ is a much better idea.

197 + def runPublishDistro(self, extra_args=None):
198 + """Invoke the publish-distro script to create indexes.

As discussed last night, ew ew ew die. But OK.

216 + def getMailRecipients(self):
217 + """List email addresses to notify of success or failure."""
218 + recipient = self.distroseries.driver or self.distribution.owner

Both have a driver and an owner. Why driver of one and owner of the other?

223 + subject = "Launchpad has created archive indexes for %s." % (
224 + self.distroseries.name)

Email subjects don't tend to have full stops, and this doesn't identify the distribution. I'd go more along the lines of "Archive indices created for Ubuntu Oneiric". This whole thing also skips all our message rationale stuff, and it all seems like a fairly dubious idea, but I guess it will have to do for now.

Also using displayname and title here... distribution.name + series.name is probably better.

304 +class TestHelpers(TestCaseWithFactory):
305 + """Test module's helpers."""

As above, this is all duplicated elsewhere. Please use the existing functions if possible.

601 + CREATEDISTROSERIESINDEXES = DBItem(4, """

That's bordering on unreadable... maybe underscores are a good idea?

Looks mostly good, but a few comments:

43	+[create_distroseries_indexes]
44	+# The database role that these jobs will run in.
45	+# datatype: string
46	+dbuser: archivepublisher

Can you change JobCronScript to take the dbuser directly, rather than continuing the proliferation of pointless config sections?

79	+        :param distroseries: A newly created `DistroSeries` that needs
80	+            its archive indexes created.

The series may have been created some time ago; it was merely recently initialised.

135	+def get_addresses_for(person):
136	+    """Get list of contact email address(es) for `person`."""

How does this differ from lp.registry.model.person.get_recipients?

145	+class CreateDistroSeriesIndexesJob(DistributionJobDerived):
146	+    """Job to create a distroseries's archive indexes.
147	+
148	+    To do this it runs publish-distro on the distribution, in careful mode.
149	+    """

I'd prefer clarification that it operates only on the primary and partner archives, and only in careful *indices* mode.

190	+        return "initializing archive indexes for %s" % self.distroseries.title

DistroSeries.title is pretty much useless... even __str__ is a much better idea.

197	+    def runPublishDistro(self, extra_args=None):
198	+        """Invoke the publish-distro script to create indexes.

As discussed last night, ew ew ew die. But OK.

216	+    def getMailRecipients(self):
217	+        """List email addresses to notify of success or failure."""
218	+        recipient = self.distroseries.driver or self.distribution.owner

Both have a driver and an owner. Why driver of one and owner of the other?

223	+        subject = "Launchpad has created archive indexes for %s." % (
224	+            self.distroseries.name)

Also using displayname and title here... distribution.name + series.name is probably better.

304	+class TestHelpers(TestCaseWithFactory):
305	+    """Test module's helpers."""

As above, this is all duplicated elsewhere. Please use the existing functions if possible.

601	+    CREATEDISTROSERIESINDEXES = DBItem(4, """

That's bordering on unreadable... maybe underscores are a good idea?

review: Approve (code*)

« Back to merge proposal