Merge lp:~barry/aptdaemon/lp1120322 into lp:aptdaemon

[Barry Warsaw]

Proposed fix for UnboundLocalError.

[lp:~barry/aptdaemon/lp1120322]

891. By Barry Warsaw on 2013-02-28

After better analysis by lifeless, fix LP: #1120322 in a complementary way:

- Improve the REGEX_RANGE to allow for optional ,### parts, so that one line
  diffs will properly match.

- Move the constant to the module globals for better wrapping (and because
  being a class instance isn't necessary).

- Still initialize line_number to 0 because it can't hurt.

- Be explicit about closing the file objects instead of hoping that gc does
  its job. This eliminates warnings in the test suite.

- Add a test for both one line and multiline diffs, showing that LP: #1120322
  is actually fixed, and doesn't regress.

[Michael Vogt (mvo)]

This looks good, thanks Barry!

review: Approve

[Michael Vogt (mvo)]

There is a issue however, the code tries to be py2 compatible as well to make e.g. backporting easier.
And e.g.:

    print('one', file=f)
                     ^
SyntaxError: invalid syntax

Is not py2 compatible. Its ultimately the decision of glatzor for how long he wants to keep py2
compatibility but in this case it seems easy enough to add.

Out of curiosity, why do you move the regexp out of the class?

review: Needs Information

[lp:~barry/aptdaemon/lp1120322]

892. By Barry Warsaw on 2013-03-01

Restore Python 2 compatibility in the tests. Document that the tests still
need to be Python 2 compatible.

[Barry Warsaw (barry)]

On Mar 01, 2013, at 09:05 AM, Michael Vogt wrote:

>There is a issue however, the code tries to be py2 compatible as well to make
>e.g. backporting easier. And e.g.:
>
> print('one', file=f)
> ^
>SyntaxError: invalid syntax
>
>Is not py2 compatible. Its ultimately the decision of glatzor for how long he
>wants to keep py2 compatibility but in this case it seems easy enough to add.

Ah darn, I forgot about Python 2 compatibility. I just pushed an update to
the branch which restores that for the test, and which describes the need to
run against Python 2 in the README.test.

>Out of curiosity, why do you move the regexp out of the class?

Purely cosmetic - I hate long lines and the changes to the regexp pushed it
past the PEP 8 line length limit. I didn't want to artificially wrap the
string, and then I noticed that there's no technical reason why it has to be a
class attribute. I can revert that if you hate it. :)

[Michael Vogt (mvo)]

On Fri, Mar 01, 2013 at 04:31:21PM -0000, Barry Warsaw wrote:
> On Mar 01, 2013, at 09:05 AM, Michael Vogt wrote:
>
> >There is a issue however, the code tries to be py2 compatible as well to make
> >e.g. backporting easier. And e.g.:
> >
> > print('one', file=f)
> > ^
> >SyntaxError: invalid syntax
> >
> >Is not py2 compatible. Its ultimately the decision of glatzor for how long he
> >wants to keep py2 compatibility but in this case it seems easy enough to add.
>
> Ah darn, I forgot about Python 2 compatibility. I just pushed an update to
> the branch which restores that for the test, and which describes the need to
> run against Python 2 in the README.test.

Thank you very much! Looks good now, merged :)

> >Out of curiosity, why do you move the regexp out of the class?
>
> Purely cosmetic - I hate long lines and the changes to the regexp pushed it
> past the PEP 8 line length limit. I didn't want to artificially wrap the
> string, and then I noticed that there's no technical reason why it has to be a
> class attribute. I can revert that if you hate it. :)

No, its fine with me, I was just curious if there is/was a reason that I
am unaware of. PEP8 is a good reason.

Cheers,
 Michael

=== modified file 'README.tests'
--- README.tests	2012-05-11 00:46:44 +0000
+++ README.tests	2013-03-01 16:26:21 +0000
@@ -2,3 +2,6 @@
 
 To run the tests you can either use the "python3 setup.py test" or
 the "nosetests3" command.
+
+Since Python 2 is still supported, you should also run the tests with
+"python setup.py test" or the "nosetests" command.

=== modified file 'aptdaemon/gtk3widgets.py'
--- aptdaemon/gtk3widgets.py	2012-12-29 19:27:25 +0000
+++ aptdaemon/gtk3widgets.py	2013-03-01 16:26:21 +0000
@@ -1062,12 +1062,14 @@
         return _ExpandableDialog.run(self)
 
 
+REGEX_RANGE = "^@@ \-(?P<from_start>[0-9]+)(?:,(?P<from_context>[0-9]+))? " \
+              "\+(?P<to_start>[0-9]+)(?:,(?P<to_context>[0-9]+))? @@"
+
+
 class DiffView(Gtk.TextView):
 
     """Shows the difference between two files."""
 
-    REGEX_RANGE = "^@@ \-(?P<from_start>[0-9]+),(?P<from_context>[0-9]+) " \
-                  "\+(?P<to_start>[0-9]+),(?P<to_context>[0-9]+) @@"
     ELLIPSIS = "[…]\n"
 
     def __init__(self):
@@ -1097,8 +1099,10 @@
         """Show the difference between two files."""
         #FIXME: Use gio
         try:
-            from_lines = open(from_path).readlines()
-            to_lines = open(to_path).readlines()
+            with open(from_path) as fp:
+                from_lines = fp.readlines()
+            with open(to_path) as fp:
+                to_lines = fp.readlines()
         except IOError:
             return
 
@@ -1111,10 +1115,11 @@
             self.textbuffer.apply_tag_by_name(
                 tag, self.textbuffer.get_iter_at_offset(offset), iter)
 
+        line_number = 0
         iter = self.textbuffer.get_start_iter()
         for line in difflib.unified_diff(from_lines, to_lines, lineterm=""):
             if line.startswith("@@"):
-                match = re.match(self.REGEX_RANGE, line)
+                match = re.match(REGEX_RANGE, line)
                 if not match:
                     continue
                 line_number = int(match.group("from_start"))

=== added file 'tests/test_gtk3widgets.py'
--- tests/test_gtk3widgets.py	1970-01-01 00:00:00 +0000
+++ tests/test_gtk3widgets.py	2013-03-01 16:26:21 +0000
@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+
+"""Test gtk3widgets.py."""
+
+import os
+import codecs
+import shutil
+import tempfile
+import unittest
+
+from aptdaemon.gtk3widgets import DiffView
+
+
+class TestLP1120322(unittest.TestCase):
+
+    def setUp(self):
+        tempdir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, tempdir)
+        self.a = os.path.join(tempdir, 'a.txt')
+        self.b = os.path.join(tempdir, 'b.txt')
+        with codecs.open(self.a, 'w', encoding='utf-8') as f:
+            f.write('one\n')
+        with codecs.open(self.b, 'w', encoding='utf-8') as f:
+            f.write('onee\n')
+
+    def test_lp_1120322(self):
+        # UnboundLocalError when the diff is one line long.
+        dv = DiffView()
+        # This simply should not traceback.
+        dv.show_diff(self.a, self.b)
+
+
+class TestGoodPath(unittest.TestCase):
+
+    def setUp(self):
+        tempdir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, tempdir)
+        self.a = os.path.join(tempdir, 'a.txt')
+        self.b = os.path.join(tempdir, 'b.txt')
+        with codecs.open(self.a, 'w', encoding='utf-8') as f:
+            f.write('one\ntwo\n')
+        with codecs.open(self.b, 'w', encoding='utf-8') as f:
+            f.write('one\ntoo\n')
+
+    def test_lp_1120322(self):
+        # UnboundLocalError when the diff is multiple lines long.
+        dv = DiffView()
+        # This simply should not traceback.
+        dv.show_diff(self.a, self.b)