Code review comment for lp://qastaging/~jcsackett/launchpad/bugfix-612408

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi John.

Thanks for making this change. Creating the soyuz publication and cache data for binary package is truly difficult. I think we want to make that easier in the future, but for now, using sample data to close an oops is the best thing.

You can land your branch after you address my style concern below.

> === modified file 'lib/lp/registry/tests/test_sourcepackage.py'
> --- lib/lp/registry/tests/test_sourcepackage.py 2010-08-03 15:28:48 +0000
> +++ lib/lp/registry/tests/test_sourcepackage.py 2010-08-04 14:23:49 +0000
> @@ -226,6 +226,25 @@
> self.assertRaises(
> NoPartnerArchive, sourcepackage.get_default_archive)
>
> + def test_source_package_summary_no_releases_returns_None(self):
> + sourcepackage = self.factory.makeSourcePackage()
> + self.assertEqual(sourcepackage.summary, None)
> +
> + def test_source_package_summary_with_releases_returns_None(self):
> + sourcepackage = self.factory.makeSourcePackage()
> + self.factory.makeSourcePackageRelease(
> + sourcepackagename=sourcepackage.sourcepackagename)
> + self.assertEqual(sourcepackage.summary, None)
> +
> + def test_source_package_summary_with_binaries_returns_list(self):
> + sp = getUtility(
> + ILaunchpadCelebrities).ubuntu['warty'].getSourcePackage(
> + 'mozilla-firefox')
> + expected_summary = u'mozilla-firefox: Mozilla Firefox \
> +Web Browser\nmozilla-firefox-data: No summary available for \
> +mozilla-firefox-data in ubuntu warty.'
> + self.assertEqual(expected_summary, sp.summary)
> +

This form of wrapping is hard to read we use () to work with long strings
and we take advantage of string concatenation rules. We want to see preserve
indentation so that the code is easy to scan. In general, we avoid
the use of \ because you have to read the ending of a line to know what is
happening on another. We avoid end comments for the same reason. I think
format works:
        expected_summary = (
            u'mozilla-firefox: Mozilla Firefox Web Browser\n'
            u'mozilla-firefox-data: No summary available for '
            u'mozilla-firefox-data in ubuntu warty.')

review: Approve (code)

« Back to merge proposal