Code review comment for lp:~wgrant/launchpad/export-das-chroot

On Sat, Feb 20, 2010 at 12:50:33AM -0000, William Grant wrote:
> === modified file 'lib/lp/soyuz/doc/distroarchseries.txt'
> --- lib/lp/soyuz/doc/distroarchseries.txt 2009-10-26 18:40:04 +0000
> +++ lib/lp/soyuz/doc/distroarchseries.txt 2010-02-20 00:50:31 +0000
> @@ -344,6 +344,14 @@
> >>> print_architectures(hoary.enabled_architectures)
> The Hoary Hedgehog Release for hppa (hppa) (ppa)
>
> +The architecture also has a 'chroot_url' attribute directly referencing
> +the file.
> +
> + >>> print hoary.getDistroArchSeries('hppa').chroot_url
> + http://localhost:58000/.../filename...
> + >>> print hoary.getDistroArchSeries('i386').chroot_url
> + None

When is chroot_url None? It'd be good to document this. Also, for test
correctness, I'd also (in addition to the current test) compare
chroot_url to getChroot().http_url, so that you know that you're not
returning just any URL.

> === modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
> --- lib/lp/soyuz/interfaces/distroarchseries.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/soyuz/interfaces/distroarchseries.py 2010-02-20 00:50:31 +0000
> @@ -108,6 +108,12 @@
> Interface, # Really IArchive, circular import fixed below.
> title=_('Main Archive'),
> description=_("The main archive of the distroarchseries.")))
> + chroot_url = exported(
> + TextLine(
> + title=_("Build chroot URL"),
> + description=_(
> + "The URL to the current build chroot for this "
> + "distroarchseries.")))
>
> def updatePackageCount():
> """Update the cached binary package count for this distro arch
>
> === modified file 'lib/lp/soyuz/model/distroarchseries.py'
> --- lib/lp/soyuz/model/distroarchseries.py 2009-12-24 06:57:25 +0000
> +++ lib/lp/soyuz/model/distroarchseries.py 2010-02-20 00:50:31 +0000
> @@ -136,6 +136,13 @@
>
> return pocket_chroot.chroot
>
> + @property
> + def chroot_url(self):
> + chroot = self.getChroot()

This should have a """See `IDistroArchSeries`.""" docstring.

I'm approving this branch with these small comments resolved.

    vote approve

--
Björn Tillenius | https://launchpad.net/~bjornt