launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py	2014-07-28 01:58:49 +0000
> +++ lib/lp/archivepublisher/publishing.py	2014-07-29 23:51:52 +0000
> @@ -900,8 +900,8 @@
>          self.log.debug("Writing Index file for %s/%s/i18n" % (
>              suite, component))
>  
> -        i18n_dir = os.path.join(self._config.distsroot, suite, component,
> -                                "i18n")
> +        i18n_path = os.path.join(component, "i18n")
> +        i18n_dir = os.path.join(self._config.distsroot, suite, i18n_path)

I'd consider renaming i18n_path to something like i18n_subpath to indicate that it's not absolute. path vs. dir is a little confusing.

>          i18n_files = []
>          try:
>              for i18n_file in os.listdir(i18n_dir):
> @@ -923,13 +923,15 @@
>          i18n_index = I18nIndex()
>          for i18n_file in sorted(i18n_files):
>              entry = self._readIndexFileContents(
> -                suite, os.path.join(component, "i18n", i18n_file))
> +                suite, os.path.join(i18n_path, i18n_file))
>              if entry is None:
>                  continue
>              i18n_index.setdefault("SHA1", []).append({
>                  "sha1": hashlib.sha1(entry).hexdigest(),
>                  "name": i18n_file,
>                  "size": len(entry)})
> +            # Schedule i18n files for inclusion in the Release file.
> +            all_series_files.add(os.path.join(i18n_path, i18n_file))
>  
>          with open(os.path.join(i18n_dir, "Index"), "w") as f:
>              i18n_index.dump(f, "utf-8")
> 
> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py	2014-07-28 01:58:49 +0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py	2014-07-29 23:51:52 +0000
> @@ -1342,6 +1342,10 @@
>  
>          self.getPubSource(filecontent='Hello world')
>  
> +        # Make sure that apt-ftparchive generates i18n/Translation-en* files.
> +        ds = self.ubuntutest.getSeries('breezy-autotest')
> +        ds.include_long_descriptions = False

Why test this in testReleaseFile rather than testReleaseFileForI18n, where the i18n/Index behaviour is already tested?

> +
>          publisher.A_publish(False)
>          publisher.C_doFTPArchive(False)
>  
> @@ -1351,8 +1355,8 @@
>  
>          publisher.D_writeReleaseFiles(False)
>  
> -        release = self.parseRelease(os.path.join(
> -            self.config.distsroot, 'breezy-autotest', 'Release'))
> +        series = os.path.join(self.config.distsroot, 'breezy-autotest')
> +        release = self.parseRelease(os.path.join(series, 'Release'))
>  
>          # Primary archive distroseries Release 'Origin' contains
>          # the distribution displayname.
> @@ -1362,8 +1366,7 @@
>          self.assertEqual('ubuntutest', release['label'])
>  
>          arch_release_path = os.path.join(
> -            self.config.distsroot, 'breezy-autotest',
> -            'main', 'source', 'Release')
> +            series, 'main', 'source', 'Release')
>          with open(arch_release_path) as arch_release_file:
>              self.assertReleaseContentsMatch(
>                  release, 'main/source/Release', arch_release_file.read())
> @@ -1373,6 +1376,18 @@
>          arch_release = self.parseRelease(arch_release_path)
>          self.assertEqual('ubuntutest', arch_release['origin'])
>  
> +        main_i18n = 'main/i18n/Translation-en.bz2'
> +        universe_i18n = 'universe/i18n/Translation-en.bz2'
> +        multiverse_i18n = 'multiverse/i18n/Translation-en.bz2'
> +        restricted_i18n = 'multiverse/i18n/Translation-en.bz2'
> +        release_path = os.path.join(series, 'Release')
> +        with open(release_path) as release_file:
> +            content = release_file.read()
> +            self.assertIn(main_i18n, content)
> +            self.assertIn(universe_i18n, content)
> +            self.assertIn(multiverse_i18n, content)
> +            self.assertIn(restricted_i18n, content)

A perfect application for a for loop.

> +
>      def testReleaseFileForPPA(self):
>          """Test release file writing for PPA
>  
> @@ -1684,7 +1699,7 @@
>                           i18n_index['sha1'][0]['size'])
>  
>          # i18n/Index is scheduled for inclusion in Release.
> -        self.assertEqual(1, len(all_files))
> +        self.assertEqual(2, len(all_files))
>          self.assertEqual(
>              os.path.join('main', 'i18n', 'Index'), all_files.pop())

This should mention and check the other file as well, rather than leaving a mysterious unnamed additional file to be guessed about.

>  
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/1313576-release-hashes/+merge/228744
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.