Merge lp:~jelmer/launchpad/635591-sync-source-unicode into lp:launchpad

Proposed by Jelmer Vernooij
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merge reported by: Jelmer Vernooij
Merged at revision: not available
Proposed branch: lp:~jelmer/launchpad/635591-sync-source-unicode
Merge into: lp:launchpad
Diff against target: 63 lines (+28/-1)
2 files modified
lib/lp/soyuz/scripts/ftpmaster.py (+1/-1)
lib/lp/soyuz/scripts/tests/test_sync_source.py (+27/-0)
To merge this branch: bzr merge lp:~jelmer/launchpad/635591-sync-source-unicode
Reviewer Review Type Date Requested Status
Gavin Panella (community) code Approve
Review via email: mp+35637@code.launchpad.net

Commit message

Fix handling of non-UTF8 changelog entries in sync-source.

Description of the change

This branch fixes the ability to sync sources that contain a changelog entry with non-ASCII UTF8 characters.

I've moved generate_changes() so that it would be possible to write unit tests for it.

tests: ./bin/test lp.soyuz.scripts.test_sync_source

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Nice branch :)

A few comments, but they're all minor. +1

[1]

There's some trailing whitespace in ftpmaster.py. There are some other
lint warnings (especially from pep8) but I don't know if any of them
are related to your changes. If you have time, consider fixing them,
but don't worry too much.

[2]

+ # Strip trailing newline
+ return changes

I think that comment can go.

[3]

+ self.assertFalse("Description" in changes)
+ self.assertFalse("Closes" in changes)
+ self.assertFalse("Launchpad-bugs-fixed" in changes)
...
+ self.assertFalse("Launchpad-bugs-fixed" in changes)

If you were to switch the base class from unittest.TestCase to
lp.testing.TestCase (or testtools.TestCase, its parent) then you could
use assertNotIn() in these places.

[4]

+ self.assertTrue(
+ contents.find("Updated French translation by J\xc3\xa9lmer."))

I think this would be clearer as:

        self.assertTrue(
            "Updated French translation by J\xc3\xa9lmer." in contents)

Or, better:

        self.assertIn(
            "Updated French translation by J\xc3\xa9lmer.", contents)

[5]

     filehandle = open(output_filename, 'w')
- # XXX cprov 2007-07-03: The Soyuz .changes parser requires the extra '\n'
- filehandle.write(changes+'\n')
- filehandle.close()
+ try:
+ changes.dump(filehandle, encoding="utf-8")
+ finally:
+ filehandle.close()

Consider using "with" here.

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/soyuz/scripts/ftpmaster.py'
2--- lib/lp/soyuz/scripts/ftpmaster.py 2010-09-16 11:14:42 +0000
3+++ lib/lp/soyuz/scripts/ftpmaster.py 2010-09-16 18:46:12 +0000
4@@ -1464,7 +1464,6 @@
5 changes["Closes"] = " ".join(closes)
6 if lp_closes:
7 changes["Launchpad-bugs-fixed"] = " ".join(lp_closes)
8- changes["Changes"] = "\n%s" % changelog
9 files = []
10 for filename in dsc_files:
11 if filename in files_from_librarian:
12@@ -1477,4 +1476,5 @@
13 })
14
15 changes["Files"] = files
16+ changes["Changes"] = "\n%s" % changelog
17 return changes
18
19=== modified file 'lib/lp/soyuz/scripts/tests/test_sync_source.py'
20--- lib/lp/soyuz/scripts/tests/test_sync_source.py 2010-09-16 11:35:26 +0000
21+++ lib/lp/soyuz/scripts/tests/test_sync_source.py 2010-09-16 18:46:12 +0000
22@@ -5,7 +5,9 @@
23
24 __metaclass__ = type
25
26+from cStringIO import StringIO
27 from debian.deb822 import (
28+ Changes,
29 Deb822Dict,
30 Dsc,
31 )
32@@ -507,6 +509,31 @@
33 self.assertIn(
34 "Updated French translation by J\xc3\xa9lmer.", contents)
35
36+ def test_changelog_whitelines(self):
37+ # The changelog entry can contain empty lines, and this should not
38+ # mess up the parsing of the changes file.
39+ changelog = "* Foo\n\n\n* Bar\n.\nEntries"
40+ changes = self.generateChanges(changelog=changelog)
41+ contents = changes.dump(encoding="utf-8").encode("utf-8")
42+ # Read contents back
43+ read_changes = Changes(StringIO(
44+ changes.dump(encoding="utf-8").encode("utf-8")))
45+ self.assertEquals("\n%s" % changelog, changes['Changes'])
46+ self.assertContentEqual([
47+ 'Architecture',
48+ 'Binary',
49+ 'Changed-By',
50+ 'Changes',
51+ 'Date',
52+ 'Distribution',
53+ 'Files',
54+ 'Format',
55+ 'Maintainer',
56+ 'Origin',
57+ 'Source',
58+ 'Urgency',
59+ 'Version',
60+ ], read_changes.keys())
61
62 def test_suite():
63 return TestLoader().loadTestsFromName(__name__)