Merge lp:~james-w/pkgme/clean-maintainer into lp:pkgme

[James Westby]

Ensure the maintainer value will always be accepted by dpkg.

Hi,

Daniel reported in https://bugs.launchpad.net/bugs/1007355 that he was getting
a failure as the maintainer info in the changelog was not recognised.

The docs say that the backend should provide the info in the accepted format,
but it's pretty easy for pkgme to fix up anything that isn't in that format
to get a working package.

I don't really see a downside to this, as if the backend provides something
valid this code shouldn't change it, and if they provide something invalid
the other choice is to fail, which isn't helpful to users.

There's a drive-by here to fix a test-isolation issue I found while running
with DEBEMAIL set in the environment. If it's set and invalid then those
tests fail as they run debuild which will fail. The fix actually means that
debuild would no longer fail, but being explicit about the isolation would
seem to be a good idea.

Thanks,

James

[Daniel Holbach (dholbach)]

Well done. The code looks clean to me and it certainly fixes the issue I ran into.

[Jonathan Lange (jml)]

Looks good. Thanks.

review: Approve

=== modified file 'pkgme/info_elements.py'
--- pkgme/info_elements.py	2012-03-30 16:04:51 +0000
+++ pkgme/info_elements.py	2012-06-04 17:00:26 +0000
@@ -1,5 +1,6 @@
 import json
 import re
+import rfc822
 import os
 
 
@@ -112,6 +113,20 @@
     def get_default(cls):
         return os.environ.get('DEBEMAIL', cls.default)
 
+    @classmethod
+    def clean(cls, value):
+        name, email = rfc822.parseaddr(value)
+        if not name:
+            name = email
+        if '@' not in email:
+            email = '[e-mail]'
+        maintainer = "%s <%s>" % (name, email)
+        # Check that we get a valid result in all cases
+        name, email = rfc822.parseaddr(maintainer)
+        if name is None or email is None:
+            raise AssertionError("Invalid maintainer format: %s" % maintainer)
+        return maintainer
+
 
 class Homepage(InfoElement):
 

=== modified file 'pkgme/tests/test_info_elements.py'
--- pkgme/tests/test_info_elements.py	2012-03-26 15:06:45 +0000
+++ pkgme/tests/test_info_elements.py	2012-06-04 17:00:26 +0000
@@ -288,6 +288,35 @@
         self.useFixture(EnvironmentVariableFixture('DEBEMAIL', debemail))
         self.assertEqual(debemail, Maintainer.get_value(DictInfo({})))
 
+    def test_clean_full(self):
+        # A full name <email> is untouched by clean
+        debemail = 'Dude <[e-mail]>'
+        self.assertEqual(debemail, Maintainer.clean(debemail))
+
+    def test_clean_email(self):
+        # Just an email is duplicated
+        debemail = '[e-mail]'
+        self.assertEqual("%s <%s>" % (debemail, debemail),
+                Maintainer.clean(debemail))
+
+    def test_clean_email_in_brackets(self):
+        # An email in brackets is duplicated minus the quotes
+        debemail = '[e-mail]'
+        self.assertEqual("%s <%s>" % (debemail, debemail),
+                Maintainer.clean("<%s>" % debemail))
+
+    def test_clean_name(self):
+        # A name is given an example email address
+        debemail = 'Dude'
+        self.assertEqual("%s <[e-mail]>" % (debemail,),
+                Maintainer.clean(debemail))
+
+    def test_name_in_brackets(self):
+        # A name in brackets is given an example email address
+        debemail = 'Dude'
+        self.assertEqual("%s <[e-mail]>" % (debemail,),
+                Maintainer.clean("<%s>" % debemail))
+
 
 class TestPackageName(TestCase):
 

=== modified file 'pkgme/tests/test_package_files.py'
--- pkgme/tests/test_package_files.py	2012-06-01 15:27:15 +0000
+++ pkgme/tests/test_package_files.py	2012-06-04 17:00:26 +0000
@@ -330,7 +330,7 @@
             ControlStanzaHasField(1, "Priority", "extra"))
 
     def test_get_contents_sets_maintainer(self):
-        maintainer = self.getUniqueString()
+        maintainer = "%s <[e-mail]>" % (self.getUniqueString(),)
         args = {Maintainer.name: maintainer}
         package_file = self.get_object(args)
         self.assertThat(

=== modified file 'pkgme/tests/test_script.py'
--- pkgme/tests/test_script.py	2012-02-01 18:16:07 +0000
+++ pkgme/tests/test_script.py	2012-06-04 17:00:26 +0000
@@ -3,7 +3,10 @@
 import pkg_resources
 from StringIO import StringIO
 
-from fixtures import MonkeyPatch
+from fixtures import (
+    EnvironmentVariableFixture,
+    MonkeyPatch,
+    )
 from testtools import TestCase
 from testtools.matchers import Not
 
@@ -26,6 +29,14 @@
 class ScriptTests(TestCase):
     """Smoke tests for the top-level pkgme script."""
 
+    def setUp(self):
+        super(ScriptTests, self).setUp()
+        # The DEBEMAIL environment variable is consulted when the
+        # pacakging is written, so set it to a known value for proper
+        # isolation
+        debemail = 'Dude <[e-mail]>'
+        self.useFixture(EnvironmentVariableFixture('DEBEMAIL', debemail))
+
     def run_script(self, cwd, argv=None):
         if argv is None:
             argv = []