launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-person-remove-ensure-unicode into launchpad:master.

Commit message:
Remove uses of ensure_unicode from PersonSet

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/398605

It's deprecated, and in most cases nowadays we can simply require Unicode input instead.  `PersonSet.getByEmails` now uses `six.ensure_text` instead, since that's very commonly used in tests via `PersonSet.getByEmail`.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-person-remove-ensure-unicode into launchpad:master.

diff --git a/lib/lp/registry/interfaces/person.py b/lib/lp/registry/interfaces/person.py
index 39988df..a956d59 100644
--- a/lib/lp/registry/interfaces/person.py
+++ b/lib/lp/registry/interfaces/person.py
@@ -2425,7 +2425,7 @@ class IPersonSet(Interface):
     @operation_returns_collection_of(IPerson)
     @export_read_operation()
     @operation_for_version("beta")
-    def find(text=""):
+    def find(text=u""):
         """Return all non-merged Persons and Teams whose name, displayname or
         email address match <text>.
 
@@ -2448,7 +2448,7 @@ class IPersonSet(Interface):
     @operation_returns_collection_of(IPerson)
     @export_read_operation()
     @operation_for_version("beta")
-    def findPerson(text="", exclude_inactive_accounts=True,
+    def findPerson(text=u"", exclude_inactive_accounts=True,
                    must_have_email=False,
                    created_after=None, created_before=None):
         """Return all non-merged Persons with at least one email address whose
@@ -2481,7 +2481,7 @@ class IPersonSet(Interface):
     @operation_returns_collection_of(IPerson)
     @export_read_operation()
     @operation_for_version("beta")
-    def findTeam(text="", preload_for_api=False):
+    def findTeam(text=u"", preload_for_api=False):
         """Return all Teams whose name, displayname or email address
         match <text>.
 
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 91ec1a6..ec5226d 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -262,7 +262,6 @@ from lp.services.database.stormbase import StormBase
 from lp.services.database.stormexpr import fti_search
 from lp.services.helpers import (
     backslashreplace,
-    ensure_unicode,
     shortlist,
     )
 from lp.services.identity.interfaces.account import (
@@ -3653,7 +3652,7 @@ class PersonSet:
             Person.teamowner != None,
             Person.merged == None,
             EmailAddress.person == Person.id,
-            EmailAddress.email.lower().startswith(ensure_unicode(text)))
+            EmailAddress.email.lower().startswith(text))
         return team_email_query
 
     def _teamNameQuery(self, text):
@@ -3664,14 +3663,13 @@ class PersonSet:
             fti_search(Person, text))
         return team_name_query
 
-    def find(self, text=""):
+    def find(self, text=u""):
         """See `IPersonSet`."""
         if not text:
             # Return an empty result set.
             return EmptyResultSet()
 
         orderBy = Person._sortingColumnsForSetOperations
-        text = ensure_unicode(text)
         lower_case_text = text.lower()
         # Teams may not have email addresses, so we need to either use a LEFT
         # OUTER JOIN or do a UNION between four queries. Using a UNION makes
@@ -3713,11 +3711,10 @@ class PersonSet:
         return results.order_by(orderBy)
 
     def findPerson(
-            self, text="", exclude_inactive_accounts=True,
+            self, text=u"", exclude_inactive_accounts=True,
             must_have_email=False, created_after=None, created_before=None):
         """See `IPersonSet`."""
         orderBy = Person._sortingColumnsForSetOperations
-        text = ensure_unicode(text)
         store = IStore(Person)
         base_query = And(
             Person.teamowner == None,
@@ -3765,10 +3762,9 @@ class PersonSet:
         combined_results = email_results.union(name_results)
         return combined_results.order_by(orderBy)
 
-    def findTeam(self, text="", preload_for_api=False):
+    def findTeam(self, text=u"", preload_for_api=False):
         """See `IPersonSet`."""
         orderBy = Person._sortingColumnsForSetOperations
-        text = ensure_unicode(text)
         # Teams may not have email addresses, so we need to either use a LEFT
         # OUTER JOIN or do a UNION between two queries. Using a UNION makes
         # it a lot faster than with a LEFT OUTER JOIN.
@@ -3803,8 +3799,7 @@ class PersonSet:
         if not emails:
             return EmptyResultSet()
         addresses = [
-            ensure_unicode(address.lower().strip())
-            for address in emails]
+            six.ensure_text(address).lower().strip() for address in emails]
         hidden_query = True
         filter_query = True
         if not include_hidden:
diff --git a/lib/lp/registry/tests/test_personset.py b/lib/lp/registry/tests/test_personset.py
index 02f8143..8fa87f0 100644
--- a/lib/lp/registry/tests/test_personset.py
+++ b/lib/lp/registry/tests/test_personset.py
@@ -239,7 +239,7 @@ class TestPersonSet(TestCaseWithFactory):
         # PersonSet.find() allows to search for OR combined terms.
         person_one = self.factory.makePerson(name='baz')
         person_two = self.factory.makeTeam(name='blah')
-        result = list(self.person_set.find('baz OR blah'))
+        result = list(self.person_set.find(u'baz OR blah'))
         self.assertEqual([person_one, person_two], result)
 
     def test_findPerson__accepts_queries_with_or_operator(self):
@@ -248,11 +248,11 @@ class TestPersonSet(TestCaseWithFactory):
             name='baz', email='one@xxxxxxxxxxx')
         person_two = self.factory.makePerson(
             name='blah', email='two@xxxxxxxxxxx')
-        result = list(self.person_set.findPerson('baz OR blah'))
+        result = list(self.person_set.findPerson(u'baz OR blah'))
         self.assertEqual([person_one, person_two], result)
         # Note that these OR searches do not work for email addresses.
         result = list(self.person_set.findPerson(
-            'one@xxxxxxxxxxx OR two@xxxxxxxxxxx'))
+            u'one@xxxxxxxxxxx OR two@xxxxxxxxxxx'))
         self.assertEqual([], result)
 
     def test_findPerson__case_insensitive_email_address_search(self):
@@ -261,29 +261,29 @@ class TestPersonSet(TestCaseWithFactory):
             name='baz', email='ONE@xxxxxxxxxxx')
         person_two = self.factory.makePerson(
             name='blah', email='two@xxxxxxxxxxx')
-        result = list(self.person_set.findPerson('one@xxxxxxxxxxx'))
+        result = list(self.person_set.findPerson(u'one@xxxxxxxxxxx'))
         self.assertEqual([person_one], result)
-        result = list(self.person_set.findPerson('TWO@xxxxxxxxxxx'))
+        result = list(self.person_set.findPerson(u'TWO@xxxxxxxxxxx'))
         self.assertEqual([person_two], result)
 
     def test_findTeam__accepts_queries_with_or_operator(self):
         # PersonSet.findTeam() allows to search for OR combined terms.
         team_one = self.factory.makeTeam(name='baz', email='ONE@xxxxxxxxxxx')
         team_two = self.factory.makeTeam(name='blah', email='TWO@xxxxxxxxxxx')
-        result = list(self.person_set.findTeam('baz OR blah'))
+        result = list(self.person_set.findTeam(u'baz OR blah'))
         self.assertEqual([team_one, team_two], result)
         # Note that these OR searches do not work for email addresses.
         result = list(self.person_set.findTeam(
-            'one@xxxxxxxxxxx OR two@xxxxxxxxxxx'))
+            u'one@xxxxxxxxxxx OR two@xxxxxxxxxxx'))
         self.assertEqual([], result)
 
     def test_findTeam__case_insensitive_email_address_search(self):
         # A search for email addresses is case insensitve.
         team_one = self.factory.makeTeam(name='baz', email='ONE@xxxxxxxxxxx')
         team_two = self.factory.makeTeam(name='blah', email='TWO@xxxxxxxxxxx')
-        result = list(self.person_set.findTeam('one@xxxxxxxxxxx'))
+        result = list(self.person_set.findTeam(u'one@xxxxxxxxxxx'))
         self.assertEqual([team_one], result)
-        result = list(self.person_set.findTeam('TWO@xxxxxxxxxxx'))
+        result = list(self.person_set.findTeam(u'TWO@xxxxxxxxxxx'))
         self.assertEqual([team_two], result)
 
 
diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
index 008dcbc..7675f9c 100644
--- a/lib/lp/registry/vocabularies.py
+++ b/lib/lp/registry/vocabularies.py
@@ -242,7 +242,6 @@ class BasePersonVocabulary:
         If the token contains an '@', treat it like an email. Otherwise,
         treat it like a name.
         """
-        token = ensure_unicode(token)
         if "@" in token:
             # This looks like an email token, so let's do an object
             # lookup based on that.
@@ -471,7 +470,7 @@ class NonMergedPeopleAndTeamsVocabulary(
         if not text:
             return self.emptySelectResults()
 
-        return self._select(ensure_unicode(text))
+        return self._select(text)
 
 
 @implementer(IHugeVocabulary)
@@ -491,7 +490,7 @@ class PersonAccountToMergeVocabulary(
     def __contains__(self, obj):
         return obj in self._select()
 
-    def _select(self, text=""):
+    def _select(self, text=u""):
         """Return `IPerson` objects that match the text."""
         return getUtility(IPersonSet).findPerson(
             text, exclude_inactive_accounts=False,
@@ -505,7 +504,6 @@ class PersonAccountToMergeVocabulary(
         if not text:
             return self.emptySelectResults()
 
-        text = ensure_unicode(text)
         return self._select(text)
 
 
@@ -739,7 +737,6 @@ class ValidPersonOrTeamVocabulary(
             else:
                 return self.emptySelectResults()
 
-        text = ensure_unicode(text)
         return self._doSearch(text=text, vocab_filter=vocab_filter)
 
     def searchForTerms(self, query=None, vocab_filter=None):