Code review comment for lp:~jcsackett/launchpad/redirects-681034

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Jonathan,
thank you for this improvement. The code looks good, I just have a little
suggestion and a question.

 review approve code

Cheers,
Henning

Am 10.12.2010 22:18, schrieb j.c.sackett:
> === modified file 'lib/lp/registry/scripts/distributionmirror_prober.py'
> --- lib/lp/registry/scripts/distributionmirror_prober.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/registry/scripts/distributionmirror_prober.py 2010-12-10 21:18:14 +0000

[...]

> @@ -540,7 +552,12 @@
>
> class MirrorCDImageProberCallbacks(LoggingMixin):
>
> - expected_failures = (BadResponseCode, ProberTimeout, ConnectionSkipped)
> + expected_failures = (
> + BadResponseCode,
> + ProberTimeout,
> + ConnectionSkipped,
> + UnknownURLSchemeAfterRedirect,
> + )

Thanks for doing the reformatting. AFAICT the order of the content of
expected_failures is irrelevant, so you could as well order them
alphabetically, like imports.

>
> def __init__(self, mirror, distroseries, flavour, log_file):
> self.mirror = mirror
> @@ -735,4 +752,3 @@
> assert port.isdigit()
> port = int(port)
> return scheme, host, port, path
> -
>
> === modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
> --- lib/lp/registry/tests/test_distributionmirror_prober.py 2010-11-22 13:55:32 +0000
> +++ lib/lp/registry/tests/test_distributionmirror_prober.py 2010-12-10 21:18:14 +0000

[...]

> @@ -651,10 +650,16 @@
> # some times.
> self.failUnlessEqual(
> set(self.callbacks.expected_failures),
> - set([BadResponseCode, ProberTimeout, ConnectionSkipped]))
> + set([
> + BadResponseCode,
> + ProberTimeout,
> + ConnectionSkipped,
> + UnknownURLSchemeAfterRedirect,
> + ]))

I realize that this is twsited-related code, so there may be a reason why
LaunchpadTestCase(WithFactory) is not use. With it, you could use cool stuff
like "assertContentEqual" and could do without the "set" trick. So you know a
reason that this test uses unittest.TestCase?

> exceptions = [BadResponseCode(str(httplib.NOT_FOUND)),
> ProberTimeout('http://localhost/', 5),
> - ConnectionSkipped()]
> + ConnectionSkipped(),
> + UnknownURLSchemeAfterRedirect('https://localhost')]
> for exception in exceptions:
> failure = self.callbacks.ensureOrDeleteMirrorCDImageSeries(
> [(defer.FAILURE, Failure(exception))])

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk0GON0ACgkQBT3oW1L17ijBWACgiEnQu82vvgPQQRqY56erjlpg
QY4An3xYVbKokM0kHn54TeYcRy49pZdh
=egAt
-----END PGP SIGNATURE-----