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
=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py 2010-09-16 11:14:42 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py 2010-09-16 18:46:12 +0000
@@ -1464,7 +1464,6 @@
1464 changes["Closes"] = " ".join(closes)1464 changes["Closes"] = " ".join(closes)
1465 if lp_closes:1465 if lp_closes:
1466 changes["Launchpad-bugs-fixed"] = " ".join(lp_closes)1466 changes["Launchpad-bugs-fixed"] = " ".join(lp_closes)
1467 changes["Changes"] = "\n%s" % changelog
1468 files = []1467 files = []
1469 for filename in dsc_files:1468 for filename in dsc_files:
1470 if filename in files_from_librarian:1469 if filename in files_from_librarian:
@@ -1477,4 +1476,5 @@
1477 })1476 })
14781477
1479 changes["Files"] = files1478 changes["Files"] = files
1479 changes["Changes"] = "\n%s" % changelog
1480 return changes1480 return changes
14811481
=== modified file 'lib/lp/soyuz/scripts/tests/test_sync_source.py'
--- lib/lp/soyuz/scripts/tests/test_sync_source.py 2010-09-16 11:35:26 +0000
+++ lib/lp/soyuz/scripts/tests/test_sync_source.py 2010-09-16 18:46:12 +0000
@@ -5,7 +5,9 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8from cStringIO import StringIO
8from debian.deb822 import (9from debian.deb822 import (
10 Changes,
9 Deb822Dict,11 Deb822Dict,
10 Dsc,12 Dsc,
11 )13 )
@@ -507,6 +509,31 @@
507 self.assertIn(509 self.assertIn(
508 "Updated French translation by J\xc3\xa9lmer.", contents)510 "Updated French translation by J\xc3\xa9lmer.", contents)
509511
512 def test_changelog_whitelines(self):
513 # The changelog entry can contain empty lines, and this should not
514 # mess up the parsing of the changes file.
515 changelog = "* Foo\n\n\n* Bar\n.\nEntries"
516 changes = self.generateChanges(changelog=changelog)
517 contents = changes.dump(encoding="utf-8").encode("utf-8")
518 # Read contents back
519 read_changes = Changes(StringIO(
520 changes.dump(encoding="utf-8").encode("utf-8")))
521 self.assertEquals("\n%s" % changelog, changes['Changes'])
522 self.assertContentEqual([
523 'Architecture',
524 'Binary',
525 'Changed-By',
526 'Changes',
527 'Date',
528 'Distribution',
529 'Files',
530 'Format',
531 'Maintainer',
532 'Origin',
533 'Source',
534 'Urgency',
535 'Version',
536 ], read_changes.keys())
510537
511def test_suite():538def test_suite():
512 return TestLoader().loadTestsFromName(__name__)539 return TestLoader().loadTestsFromName(__name__)