Code review comment for lp:~jtv/launchpad/bug-824499

> review approve

Thanks again. The notes one by one:

> > > === modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
> > > --- lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > +++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > @@ -31,12 +31,20 @@

> > > + return [
> > > + archive
> > > + for archive in distribution.all_distro_archives
> > > + if archive.purpose in ARCHIVES_TO_PUBLISH]
>
> I don't know if you like lambdas but they are not banned anymore
> AFAIK. So this could be written as:
>
> return filter(
> distribution.all_distro_archives,
> lambda archive: archive.purpose in ARCHIVES_TO_PUBLISH)
>
> But I let you decide what you find more readable. I personally do not
> prefer the dangling if clause.

Neither do I, though in this case I feel a very slight preference for the "if" because it reads so naturally.

> > > + self.distributions = distro_set.getDerivedDistributions()
>
> Should the script not say something if this list is empty? Or even
> refuse to start?

Well, there is already a check that a self.distributions has to be selected one way or the other. After that, self.distributions could only be empty if the --all-derived option was passed but there are no derived distributions.

Since this will be running from a cron job, I think we probably shouldn't be generating repeated oopses just because there's no derived distro yet. Or if all derived distros disappear from the database later, I'd like to think that we'd find out about it in other ways. :)

Refusing to start probably wouldn't do us much good: the script would iterate over zero distributions anyway.

> > > === modified file
> > > 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
> > > --- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > +++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > @@ -290,6 +290,22 @@

> > > + reference_config = getPubConfig(distro.main_archive)
> > > + config = script.getConfigs()[distro][ArchivePurpose.PRIMARY]
> > > + self.assertEqual(reference_config.temproot, config.temproot)
> > > + self.assertEqual(reference_config.distroroot,
> > > config.distroroot)
> > > + self.assertEqual(reference_config.archiveroot,
> > > config.archiveroot)
>
> This is a Good Place for the MatchesStructure matcher from testtools.

Yes, it is. Kudos for spotting that it's not in testtools.matchers.__all__ (thus explaining why I didn't find it in __all__). I'll rewrite.

Jeroen