Merge lp:~jtv/launchpad/exportstorage-cleanup into lp:launchpad

[Jeroen T. Vermeulen]

Set translations export storage MIME type when adding file, not on __init__.

= Export Storage Strategy cleanup =

This is preparation for an overhaul of the translations export machinery.

Sometimes the export needs to store a single file. Sometimes it needs to store several, which have to be bundled up into a tarball first. A neat little "strategy" pattern hides this inside an object that just accepts files.

Currently the storage strategy is made to contain files of a single MIME type. This fits the use cases—we actually don't care about MIME types inside a tarball!—but not the use pattern once we start dumping multiple files into one storage object.

In this branch I provide the MIME type not while creating the storage object, but when adding a file. To test,
{{{
./bin/test -vvc -m lp.translations.utilities -t Storage
}}}

No lint.

Jeroen

[Robert Collins (lifeless)]

Looks good - thanks.

review: Approve

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/translations/utilities/gettext_mo_exporter.py'
--- lib/lp/translations/utilities/gettext_mo_exporter.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/gettext_mo_exporter.py	2010-07-20 14:50:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Export module for gettext's .mo file format."""
@@ -7,7 +7,7 @@
 
 __all__ = [
     'GettextMOExporter',
-    'POCompiler'
+    'POCompiler',
     ]
 
 import os
@@ -70,7 +70,8 @@
             translation_exporter.getExporterProducingTargetFileFormat(
                 TranslationFileFormat.PO))
 
-        storage = ExportFileStorage('application/x-gmo')
+        mime_type = 'application/x-gmo'
+        storage = ExportFileStorage()
 
         for translation_file in translation_files:
             # To generate MO files we need first its PO version and then,
@@ -103,7 +104,7 @@
                 # GTranslator.
                 content_type = 'application/x-gmo'
 
-            storage.addFile(file_path, file_extension, exported_file_content)
+            storage.addFile(
+                file_path, file_extension, exported_file_content, mime_type)
 
         return storage.export()
-

=== modified file 'lib/lp/translations/utilities/gettext_po_exporter.py'
--- lib/lp/translations/utilities/gettext_po_exporter.py	2010-01-14 09:07:00 +0000
+++ lib/lp/translations/utilities/gettext_po_exporter.py	2010-07-20 14:50:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Export module for gettext's .po file format.
@@ -39,6 +39,7 @@
     else:
         return text
 
+
 def comments_text_representation(translation_message):
     """Return text representation of the comments.
 
@@ -103,6 +104,7 @@
         it.
     :param wrap_width: The width where the text should be wrapped.
     """
+
     def local_escape(text):
         ret = text.replace(u'\\', u'\\\\')
         ret = ret.replace(ur'"', ur'\"')
@@ -297,7 +299,8 @@
     def exportTranslationFiles(self, translation_files, ignore_obsolete=False,
                                force_utf8=False):
         """See `ITranslationFormatExporter`."""
-        storage = ExportFileStorage('application/x-po')
+        mime_type = 'application/x-po'
+        storage = ExportFileStorage()
 
         for translation_file in translation_files:
             dirname = os.path.dirname(translation_file.path)
@@ -377,7 +380,8 @@
                 encoded_file_content = self._encode_file_content(
                     translation_file, exported_file_content)
 
-            storage.addFile(file_path, file_extension, encoded_file_content)
+            storage.addFile(
+                file_path, file_extension, encoded_file_content, mime_type)
 
         return storage.export()
 

=== modified file 'lib/lp/translations/utilities/tests/test_export_file_storage.py'
--- lib/lp/translations/utilities/tests/test_export_file_storage.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/tests/test_export_file_storage.py	2010-07-20 14:50:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `ExportFileStorage`."""
@@ -19,7 +19,8 @@
 
     def testEmpty(self):
         """Behaviour of empty storage."""
-        storage = ExportFileStorage('application/x-po')
+        mime = 'application/x-po'
+        storage = ExportFileStorage()
         # Try not inserting any files, so the storage object remains empty.
         self.assertTrue(storage._store.isEmpty())
         self.assertFalse(storage._store.isFull())
@@ -28,25 +29,29 @@
 
     def testFull(self):
         """Behaviour of isFull."""
-        storage = ExportFileStorage('application/x-po')
-        storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
+        mime = 'application/x-po'
+        storage = ExportFileStorage()
+        storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
         # The storage object starts out with a SingleFileStorageStrategy, so
         # it's full now that we've added one file.
         self.assertTrue(storage._store.isFull())
         # If we add another file however, the storage object transparently
         # switches to a TarballFileStorageStrategy.  That type of storage
         # object is never full.
-        storage.addFile('/tmp/another/test/file.po', 'po', 'test file two')
+        storage.addFile(
+            '/tmp/another/test/file.po', 'po', 'test file two', mime)
         self.assertFalse(storage._store.isFull())
         # We can now add any number of files without filling the storage
         # object.
-        storage.addFile('/tmp/yet/another/test/file.po', 'po', 'test file 3')
+        storage.addFile(
+            '/tmp/yet/another/test/file.po', 'po', 'test file 3', mime)
         self.assertFalse(storage._store.isFull())
 
     def testSingle(self):
         """Test export of single file."""
-        storage = ExportFileStorage('application/x-po')
-        storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
+        mime = 'application/x-po'
+        storage = ExportFileStorage()
+        storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
         outfile = storage.export()
         self.assertEquals(outfile.path, '/tmp/a/test/file.po')
         self.assertEquals(outfile.file_extension, 'po')
@@ -54,9 +59,11 @@
 
     def testTarball(self):
         """Test export of tarball."""
-        storage = ExportFileStorage('application/x-po')
-        storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
-        storage.addFile('/tmp/another/test.po', 'po', 'another test file')
+        mime = 'application/x-po'
+        storage = ExportFileStorage()
+        storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
+        storage.addFile(
+            '/tmp/another/test.po', 'po', 'another test file', mime)
         outfile = storage.export()
         tarball = TarFile.open(mode='r|gz', fileobj=StringIO(outfile.read()))
         elements = set(tarball.getnames())
@@ -68,4 +75,3 @@
     suite = unittest.TestSuite()
     suite.addTest(unittest.makeSuite(ExportFileStorageTestCase))
     return suite
-

=== modified file 'lib/lp/translations/utilities/translation_export.py'
--- lib/lp/translations/utilities/translation_export.py	2009-10-13 17:25:19 +0000
+++ lib/lp/translations/utilities/translation_export.py	2010-07-20 14:50:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Components for exporting translation files."""
@@ -27,12 +27,13 @@
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
 
+
 class ExportedTranslationFile:
     """See `IExportedTranslationFile`."""
     implements(IExportedTranslationFile)
 
     def __init__(self, content_file):
-        self._content_file  = content_file
+        self._content_file = content_file
         self.content_type = None
         self.path = None
         self.file_extension = None
@@ -178,7 +179,8 @@
     Storage for single files is implemented by `SingleFileStorageStrategy`;
     multiple files go into a `TarballFileStorageStrategy`.
     """
-    def addFile(self, path, extension, content):
+
+    def addFile(self, path, extension, content, content_type):
         """Add a file to be stored."""
         raise NotImplementedError()
 
@@ -208,18 +210,15 @@
 
     path = None
     extension = None
-    mimetype = None
-
-    def __init__(self, mimetype):
-        self.mimetype = mimetype
-
-    def addFile(self, path, extension, content):
+
+    def addFile(self, path, extension, content, mime_type):
         """See `StorageStrategy`."""
         assert path is not None, "Storing file without path."
         assert self.path is None, "Multiple files added; expected just one."
         self.path = path
         self.extension = extension
         self.content = content
+        self.mime_type = mime_type
 
     def isEmpty(self):
         """See `StorageStrategy`."""
@@ -238,7 +237,7 @@
         output = ExportedTranslationFile(StringIO(self.content))
         output.path = self.path
         # We use x-po for consistency with other .po editors like GTranslator.
-        output.content_type = self.mimetype
+        output.content_type = self.mime_type
         output.file_extension = self.extension
         return output
 
@@ -251,6 +250,8 @@
     as soon as it is added.  There is no need to keep the full contents of the
     tarball in memory at any single time.
     """
+    mime_type = 'application/x-gtar'
+
     empty = False
 
     def __init__(self, single_file_storage=None):
@@ -258,11 +259,13 @@
         self.buffer = tempfile.TemporaryFile()
         self.tar_writer = LaunchpadWriteTarFile(self.buffer)
         if single_file_storage is not None:
-            self.addFile(single_file_storage.path,
-                single_file_storage.extension, single_file_storage.content)
+            self.addFile(
+                single_file_storage.path, single_file_storage.extension,
+                single_file_storage.content, single_file_storage.mime_type)
 
-    def addFile(self, path, extension, content):
+    def addFile(self, path, extension, content, mime_type):
         """See `StorageStrategy`."""
+        # Tarballs don't store MIME types, so ignore that.
         self.empty = False
         self.tar_writer.add_file(path, content)
 
@@ -288,19 +291,20 @@
         # For tar.gz files, the standard content type is application/x-gtar.
         # You can see more info on
         #   http://en.wikipedia.org/wiki/List_of_archive_formats
-        output.content_type = 'application/x-gtar'
+        output.content_type = self.mime_type
         output.file_extension = 'tar.gz'
         return output
 
 
 class ExportFileStorage:
     """Store files to export, either as tarball or plain single file."""
-    def __init__(self, mimetype):
+
+    def __init__(self):
         # Start out with a single file.  We can replace that strategy later if
         # we get more than one file.
-        self._store = SingleFileStorageStrategy(mimetype)
+        self._store = SingleFileStorageStrategy()
 
-    def addFile(self, path, extension, content):
+    def addFile(self, path, extension, content, mime_type):
         """Add file to be stored.
 
         :param path: location and name of this file, relative to root of tar
@@ -312,10 +316,9 @@
             # We're still using a single-file storage strategy, but we just
             # received our second file.  Switch to tarball strategy.
             self._store = TarballFileStorageStrategy(self._store)
-        self._store.addFile(path, extension, content)
+        self._store.addFile(path, extension, content, mime_type)
 
     def export(self):
         """Export as `ExportedTranslationFile`."""
         assert not self._store.isEmpty(), "Got empty list of files to export."
         return self._store.export()
-