Launchpad itself

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

  1. exportstorage-cleanup
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11178
Proposed branch: lp:~jtv/launchpad/exportstorage-cleanup
Merge into: lp:launchpad
Diff against target: 289 lines (+53/-39)
4 files modified
lib/lp/translations/utilities/gettext_mo_exporter.py (+6/-5)
lib/lp/translations/utilities/gettext_po_exporter.py (+7/-3)
lib/lp/translations/utilities/tests/test_export_file_storage.py (+18/-12)
lib/lp/translations/utilities/translation_export.py (+22/-19)
To merge this branch: bzr merge lp:~jtv/launchpad/exportstorage-cleanup
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Robert Collins (community) Approve
Review via email: mp+30418@code.launchpad.net

Commit message

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

Description of the change

= 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

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Looks good - thanks.

review: Approve
Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/utilities/gettext_mo_exporter.py'
2--- lib/lp/translations/utilities/gettext_mo_exporter.py 2009-07-17 00:26:05 +0000
3+++ lib/lp/translations/utilities/gettext_mo_exporter.py 2010-07-20 14:50:26 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """Export module for gettext's .mo file format."""
10@@ -7,7 +7,7 @@
11
12 __all__ = [
13 'GettextMOExporter',
14- 'POCompiler'
15+ 'POCompiler',
16 ]
17
18 import os
19@@ -70,7 +70,8 @@
20 translation_exporter.getExporterProducingTargetFileFormat(
21 TranslationFileFormat.PO))
22
23- storage = ExportFileStorage('application/x-gmo')
24+ mime_type = 'application/x-gmo'
25+ storage = ExportFileStorage()
26
27 for translation_file in translation_files:
28 # To generate MO files we need first its PO version and then,
29@@ -103,7 +104,7 @@
30 # GTranslator.
31 content_type = 'application/x-gmo'
32
33- storage.addFile(file_path, file_extension, exported_file_content)
34+ storage.addFile(
35+ file_path, file_extension, exported_file_content, mime_type)
36
37 return storage.export()
38-
39
40=== modified file 'lib/lp/translations/utilities/gettext_po_exporter.py'
41--- lib/lp/translations/utilities/gettext_po_exporter.py 2010-01-14 09:07:00 +0000
42+++ lib/lp/translations/utilities/gettext_po_exporter.py 2010-07-20 14:50:26 +0000
43@@ -1,4 +1,4 @@
44-# Copyright 2009 Canonical Ltd. This software is licensed under the
45+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
46 # GNU Affero General Public License version 3 (see the file LICENSE).
47
48 """Export module for gettext's .po file format.
49@@ -39,6 +39,7 @@
50 else:
51 return text
52
53+
54 def comments_text_representation(translation_message):
55 """Return text representation of the comments.
56
57@@ -103,6 +104,7 @@
58 it.
59 :param wrap_width: The width where the text should be wrapped.
60 """
61+
62 def local_escape(text):
63 ret = text.replace(u'\\', u'\\\\')
64 ret = ret.replace(ur'"', ur'\"')
65@@ -297,7 +299,8 @@
66 def exportTranslationFiles(self, translation_files, ignore_obsolete=False,
67 force_utf8=False):
68 """See `ITranslationFormatExporter`."""
69- storage = ExportFileStorage('application/x-po')
70+ mime_type = 'application/x-po'
71+ storage = ExportFileStorage()
72
73 for translation_file in translation_files:
74 dirname = os.path.dirname(translation_file.path)
75@@ -377,7 +380,8 @@
76 encoded_file_content = self._encode_file_content(
77 translation_file, exported_file_content)
78
79- storage.addFile(file_path, file_extension, encoded_file_content)
80+ storage.addFile(
81+ file_path, file_extension, encoded_file_content, mime_type)
82
83 return storage.export()
84
85
86=== modified file 'lib/lp/translations/utilities/tests/test_export_file_storage.py'
87--- lib/lp/translations/utilities/tests/test_export_file_storage.py 2009-07-17 00:26:05 +0000
88+++ lib/lp/translations/utilities/tests/test_export_file_storage.py 2010-07-20 14:50:26 +0000
89@@ -1,4 +1,4 @@
90-# Copyright 2009 Canonical Ltd. This software is licensed under the
91+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
92 # GNU Affero General Public License version 3 (see the file LICENSE).
93
94 """Tests for `ExportFileStorage`."""
95@@ -19,7 +19,8 @@
96
97 def testEmpty(self):
98 """Behaviour of empty storage."""
99- storage = ExportFileStorage('application/x-po')
100+ mime = 'application/x-po'
101+ storage = ExportFileStorage()
102 # Try not inserting any files, so the storage object remains empty.
103 self.assertTrue(storage._store.isEmpty())
104 self.assertFalse(storage._store.isFull())
105@@ -28,25 +29,29 @@
106
107 def testFull(self):
108 """Behaviour of isFull."""
109- storage = ExportFileStorage('application/x-po')
110- storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
111+ mime = 'application/x-po'
112+ storage = ExportFileStorage()
113+ storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
114 # The storage object starts out with a SingleFileStorageStrategy, so
115 # it's full now that we've added one file.
116 self.assertTrue(storage._store.isFull())
117 # If we add another file however, the storage object transparently
118 # switches to a TarballFileStorageStrategy. That type of storage
119 # object is never full.
120- storage.addFile('/tmp/another/test/file.po', 'po', 'test file two')
121+ storage.addFile(
122+ '/tmp/another/test/file.po', 'po', 'test file two', mime)
123 self.assertFalse(storage._store.isFull())
124 # We can now add any number of files without filling the storage
125 # object.
126- storage.addFile('/tmp/yet/another/test/file.po', 'po', 'test file 3')
127+ storage.addFile(
128+ '/tmp/yet/another/test/file.po', 'po', 'test file 3', mime)
129 self.assertFalse(storage._store.isFull())
130
131 def testSingle(self):
132 """Test export of single file."""
133- storage = ExportFileStorage('application/x-po')
134- storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
135+ mime = 'application/x-po'
136+ storage = ExportFileStorage()
137+ storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
138 outfile = storage.export()
139 self.assertEquals(outfile.path, '/tmp/a/test/file.po')
140 self.assertEquals(outfile.file_extension, 'po')
141@@ -54,9 +59,11 @@
142
143 def testTarball(self):
144 """Test export of tarball."""
145- storage = ExportFileStorage('application/x-po')
146- storage.addFile('/tmp/a/test/file.po', 'po', 'test file')
147- storage.addFile('/tmp/another/test.po', 'po', 'another test file')
148+ mime = 'application/x-po'
149+ storage = ExportFileStorage()
150+ storage.addFile('/tmp/a/test/file.po', 'po', 'test file', mime)
151+ storage.addFile(
152+ '/tmp/another/test.po', 'po', 'another test file', mime)
153 outfile = storage.export()
154 tarball = TarFile.open(mode='r|gz', fileobj=StringIO(outfile.read()))
155 elements = set(tarball.getnames())
156@@ -68,4 +75,3 @@
157 suite = unittest.TestSuite()
158 suite.addTest(unittest.makeSuite(ExportFileStorageTestCase))
159 return suite
160-
161
162=== modified file 'lib/lp/translations/utilities/translation_export.py'
163--- lib/lp/translations/utilities/translation_export.py 2009-10-13 17:25:19 +0000
164+++ lib/lp/translations/utilities/translation_export.py 2010-07-20 14:50:26 +0000
165@@ -1,4 +1,4 @@
166-# Copyright 2009 Canonical Ltd. This software is licensed under the
167+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
168 # GNU Affero General Public License version 3 (see the file LICENSE).
169
170 """Components for exporting translation files."""
171@@ -27,12 +27,13 @@
172 from lp.translations.interfaces.translationfileformat import (
173 TranslationFileFormat)
174
175+
176 class ExportedTranslationFile:
177 """See `IExportedTranslationFile`."""
178 implements(IExportedTranslationFile)
179
180 def __init__(self, content_file):
181- self._content_file = content_file
182+ self._content_file = content_file
183 self.content_type = None
184 self.path = None
185 self.file_extension = None
186@@ -178,7 +179,8 @@
187 Storage for single files is implemented by `SingleFileStorageStrategy`;
188 multiple files go into a `TarballFileStorageStrategy`.
189 """
190- def addFile(self, path, extension, content):
191+
192+ def addFile(self, path, extension, content, content_type):
193 """Add a file to be stored."""
194 raise NotImplementedError()
195
196@@ -208,18 +210,15 @@
197
198 path = None
199 extension = None
200- mimetype = None
201-
202- def __init__(self, mimetype):
203- self.mimetype = mimetype
204-
205- def addFile(self, path, extension, content):
206+
207+ def addFile(self, path, extension, content, mime_type):
208 """See `StorageStrategy`."""
209 assert path is not None, "Storing file without path."
210 assert self.path is None, "Multiple files added; expected just one."
211 self.path = path
212 self.extension = extension
213 self.content = content
214+ self.mime_type = mime_type
215
216 def isEmpty(self):
217 """See `StorageStrategy`."""
218@@ -238,7 +237,7 @@
219 output = ExportedTranslationFile(StringIO(self.content))
220 output.path = self.path
221 # We use x-po for consistency with other .po editors like GTranslator.
222- output.content_type = self.mimetype
223+ output.content_type = self.mime_type
224 output.file_extension = self.extension
225 return output
226
227@@ -251,6 +250,8 @@
228 as soon as it is added. There is no need to keep the full contents of the
229 tarball in memory at any single time.
230 """
231+ mime_type = 'application/x-gtar'
232+
233 empty = False
234
235 def __init__(self, single_file_storage=None):
236@@ -258,11 +259,13 @@
237 self.buffer = tempfile.TemporaryFile()
238 self.tar_writer = LaunchpadWriteTarFile(self.buffer)
239 if single_file_storage is not None:
240- self.addFile(single_file_storage.path,
241- single_file_storage.extension, single_file_storage.content)
242+ self.addFile(
243+ single_file_storage.path, single_file_storage.extension,
244+ single_file_storage.content, single_file_storage.mime_type)
245
246- def addFile(self, path, extension, content):
247+ def addFile(self, path, extension, content, mime_type):
248 """See `StorageStrategy`."""
249+ # Tarballs don't store MIME types, so ignore that.
250 self.empty = False
251 self.tar_writer.add_file(path, content)
252
253@@ -288,19 +291,20 @@
254 # For tar.gz files, the standard content type is application/x-gtar.
255 # You can see more info on
256 # http://en.wikipedia.org/wiki/List_of_archive_formats
257- output.content_type = 'application/x-gtar'
258+ output.content_type = self.mime_type
259 output.file_extension = 'tar.gz'
260 return output
261
262
263 class ExportFileStorage:
264 """Store files to export, either as tarball or plain single file."""
265- def __init__(self, mimetype):
266+
267+ def __init__(self):
268 # Start out with a single file. We can replace that strategy later if
269 # we get more than one file.
270- self._store = SingleFileStorageStrategy(mimetype)
271+ self._store = SingleFileStorageStrategy()
272
273- def addFile(self, path, extension, content):
274+ def addFile(self, path, extension, content, mime_type):
275 """Add file to be stored.
276
277 :param path: location and name of this file, relative to root of tar
278@@ -312,10 +316,9 @@
279 # We're still using a single-file storage strategy, but we just
280 # received our second file. Switch to tarball strategy.
281 self._store = TarballFileStorageStrategy(self._store)
282- self._store.addFile(path, extension, content)
283+ self._store.addFile(path, extension, content, mime_type)
284
285 def export(self):
286 """Export as `ExportedTranslationFile`."""
287 assert not self._store.isEmpty(), "Got empty list of files to export."
288 return self._store.export()
289-