Merge lp:~wgrant/bzr-pqm/bug-711016-no-ui-clause into lp:bzr-pqm

[William Grant]

This branch adjusts lp-land to only include a [ui=...] clause when there are UI reviewers, rather than always including [ui=none]. This same change was made to ec2 land in https://code.launchpad.net/~wgrant/launchpad/bug-711016-no-ui-clause/+merge/48094.

[Jelmer Vernooij (jelmer)]

Thanks.

As a side-note, I'm surprised there is this much project-specific code in bzr-pqm.

review: Approve

[Jelmer Vernooij (jelmer)]

If I should land this, please let me know.

[Aaron Bentley (abentley)]

About the project-specific code in lp-land-- that's because I originally wrote lp-land as a standalone plugin for use with Launchpad landings. I believe that we are one of the few users of PQM, so it seemed fine to leave in.

=== modified file 'lpland.py'
--- lpland.py	2010-11-08 16:49:24 +0000
+++ lpland.py	2011-02-01 10:38:27 +0000
@@ -386,13 +386,13 @@
     if not code_reviewers:
         raise MissingReviewError("Need approved votes in order to land.")
     if ui_reviewers:
-        ui_clause = _comma_separated_names(ui_reviewers)
+        ui_clause = '[ui=%s]' % _comma_separated_names(ui_reviewers)
     else:
-        ui_clause = 'none'
+        ui_clause = ''
     if rc_reviewers:
         rc_clause = (
             '[release-critical=%s]' % _comma_separated_names(rc_reviewers))
     else:
         rc_clause = ''
-    return '%s[r=%s][ui=%s]' % (
+    return '%s[r=%s]%s' % (
         rc_clause, _comma_separated_names(code_reviewers), ui_clause)
 
=== modified file 'tests/test_lpland.py'
--- tests/test_lpland.py	2010-11-08 16:49:24 +0000
+++ tests/test_lpland.py	2011-02-01 10:38:27 +0000
@@ -209,7 +209,7 @@
         self.mp.get_bugs = FakeMethod([self.fake_bug])
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
-        self.assertEqual("[r=foo][ui=none][bug=20] Foobaring the sbrubble.",
+        self.assertEqual("[r=foo][bug=20] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
@@ -232,7 +232,7 @@
         self.mp.get_bugs = FakeMethod([])
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
-        self.assertEqual("[r=foo][ui=none][no-qa] Foobaring the sbrubble.",
+        self.assertEqual("[r=foo][no-qa] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
@@ -245,7 +245,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][no-qa] Foobaring the sbrubble.",
+            "[r=foo][bug=20][no-qa] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
@@ -258,7 +258,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
+            "[r=foo][bug=20][incr] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
@@ -271,7 +271,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
+            "[r=foo][bug=20][incr] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
@@ -284,7 +284,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][no-qa][incr] Foobaring the sbrubble.",
+            "[r=foo][bug=20][no-qa][incr] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.", 
                 testfix, no_qa, incr))
 
@@ -293,7 +293,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
+            "[r=foo][bug=20][rollback=123] Foobaring the sbrubble.",
             self.mp.build_commit_message("Foobaring the sbrubble.", 
                 rollback=123))
 
@@ -302,9 +302,9 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual(
-            "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
+            "[r=foo][bug=20][rollback=123] Foobaring the sbrubble.",
             self.mp.build_commit_message(
-                "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
+                "[r=foo][bug=20][rollback=123] Foobaring the sbrubble.",
                 rollback=123))
 
 
@@ -331,15 +331,15 @@
     def test_one_reviewer_no_type(self):
         # It's very common for a merge proposal to be reviewed by one person
         # with no specified type of review. It such cases the review clause is
-        # '[r=<person>][ui=none]'.
+        # '[r=<person>]'.
         clause = self.get_reviewer_clause({None: [self.makePerson('foo')]})
-        self.assertEqual('[r=foo][ui=none]', clause)
+        self.assertEqual('[r=foo]', clause)
 
     def test_two_reviewers_no_type(self):
         # Branches can have more than one reviewer.
         clause = self.get_reviewer_clause(
             {None: [self.makePerson('foo'), self.makePerson('bar')]})
-        self.assertEqual('[r=bar,foo][ui=none]', clause)
+        self.assertEqual('[r=bar,foo]', clause)
 
     def test_mentat_reviewers(self):
         # A mentat review sometimes is marked like 'ui*'.  Due to the
@@ -357,7 +357,7 @@
         # reviews, these are treated in the same way as reviewers without any
         # given type.
         clause = self.get_reviewer_clause({'code': [self.makePerson('foo')]})
-        self.assertEqual('[r=foo][ui=none]', clause)
+        self.assertEqual('[r=foo]', clause)
 
     def test_release_critical(self):
         # Reviews that are marked as release-critical are included in a
@@ -365,13 +365,13 @@
         clause = self.get_reviewer_clause(
             {'code': [self.makePerson('foo')],
              'release-critical': [self.makePerson('bar')]})
-        self.assertEqual('[release-critical=bar][r=foo][ui=none]', clause)
+        self.assertEqual('[release-critical=bar][r=foo]', clause)
 
     def test_db_reviewer_counts(self):
         # There's no special way of annotating database reviews in Launchpad
         # commit messages, so they are included with the code reviews.
         clause = self.get_reviewer_clause({'db': [self.makePerson('foo')]})
-        self.assertEqual('[r=foo][ui=none]', clause)
+        self.assertEqual('[r=foo]', clause)
 
     def test_ui_reviewers(self):
         # If someone has done a UI review, then that appears in the clause