launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-diff-text into launchpad:master.

Commit message:
Make sure Diff.text returns text

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This is considerably less confusing than a property called "text" returning bytes, and it fixes some test failures on Python 3.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-diff-text into launchpad:master.

diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
index aecad03..a72cf2a 100644
--- a/lib/lp/code/browser/branchmergeproposal.py
+++ b/lib/lp/code/browser/branchmergeproposal.py
@@ -540,9 +540,7 @@ class DiffRenderingMixin:
         if preview_diff is None:
             return None
         try:
-            diff = preview_diff.text.decode('utf-8')
-        except UnicodeDecodeError:
-            diff = preview_diff.text.decode('windows-1252', 'replace')
+            diff = preview_diff.text
         except (LookupError, LibrarianServerError):
             self._diff_available = False
             diff = ''
diff --git a/lib/lp/code/mail/codereviewcomment.py b/lib/lp/code/mail/codereviewcomment.py
index 953c839..0a885d2 100644
--- a/lib/lp/code/mail/codereviewcomment.py
+++ b/lib/lp/code/mail/codereviewcomment.py
@@ -111,7 +111,8 @@ class CodeReviewCommentMailer(BMPMailer):
                 self.code_review_comment)
         if inline_comment is not None:
             self.body_main += build_inline_comments_section(
-                inline_comment.comments, inline_comment.previewdiff.text)
+                inline_comment.comments,
+                inline_comment.previewdiff.text.encode('UTF-8'))
 
         self.proposal_url = canonical_url(self.merge_proposal)
 
diff --git a/lib/lp/code/model/diff.py b/lib/lp/code/model/diff.py
index 54f1aa1..6b3ca22 100644
--- a/lib/lp/code/model/diff.py
+++ b/lib/lp/code/model/diff.py
@@ -102,7 +102,7 @@ class Diff(SQLBase):
     @property
     def text(self):
         if self.diff_text is None:
-            return ''
+            return u''
         else:
             with reduced_timeout(
                     0.01, webapp_max=2.0,
@@ -110,7 +110,13 @@ class Diff(SQLBase):
                 timeout = get_default_timeout_function()()
             self.diff_text.open(timeout)
             try:
-                return self.diff_text.read(config.diff.max_read_size)
+                diff_bytes = self.diff_text.read(config.diff.max_read_size)
+                # Attempt to decode the diff somewhat intelligently,
+                # although this may not be a great heuristic.
+                try:
+                    return diff_bytes.decode('utf-8')
+                except UnicodeDecodeError:
+                    return diff_bytes.decode('windows-1252', 'replace')
             finally:
                 self.diff_text.close()
 
diff --git a/lib/lp/code/model/tests/test_diff.py b/lib/lp/code/model/tests/test_diff.py
index 0c49746..71510b3 100644
--- a/lib/lp/code/model/tests/test_diff.py
+++ b/lib/lp/code/model/tests/test_diff.py
@@ -240,7 +240,7 @@ class TestDiff(DiffTestCase):
 
         diff = DiffWithFakeText()
         diff.diff_text.open = FakeMethod()
-        diff.diff_text.read = FakeMethod()
+        diff.diff_text.read = FakeMethod(result=b'')
         diff.diff_text.close = FakeMethod()
         value = None
         original_timeout_function = get_default_timeout_function()
@@ -735,6 +735,7 @@ class TestIncrementalDiff(DiffTestCase):
         incremental_diff = bmp.generateIncrementalDiff(
             old_revision, new_revision)
         transaction.commit()
-        inserted, removed = self.diff_changes(incremental_diff.text)
-        self.assertEqual(['c\n'], inserted)
-        self.assertEqual(['b\n'], removed)
+        inserted, removed = self.diff_changes(
+            incremental_diff.text.encode('UTF-8'))
+        self.assertEqual([b'c\n'], inserted)
+        self.assertEqual([b'b\n'], removed)