Python3 compatible!

* Replace None with '' for added/deleted files

  Let mylist = ['b', 'a', 'c', None].  In Python2 `sorted(mylist)`
  returns `[None, 'a', 'b', 'c'].  In Python3 `sorted(mylist)` errors
  out because you can't compare a `None` to strings.  Because of this
  difference I chose to replace `None` with `''` for files which have
  been added or deleted. It still makes sense logically because the path
  to the file is non-existent so `''` and the name of the file is `''`.

* Had to add mode='rb' to file `open()`

* Using binaryornot package because it handles the python2/python3
  differences in how to specify the characters to remove when checking
  if file is binary

* Some general code clean up of shadowed variables, unused
  modules/variables etc.

Author: Byron Boulton

requirements.txt

@@ -3,3 +3,4 @@ nose
 PyGithub==1.25.2
 pillow
 requests
+binaryornot

tests/pair_test.py

@@ -27,7 +27,7 @@ def test_pairing():
     pairs.sort()
 
     eq_(
-        [(None, 'testing.cfg'),
+        [('', 'testing.cfg'),
          ('TODO', 'TODO'),
          ('app.py', 'app.py'),
          ('static/js/file_diff.js', 'static/js/file_diff.js'),
@@ -47,7 +47,7 @@ def test_pairing_with_move():
          },
          {
           'a': 'file.json',
-          'b': None,
+          'b': '',
           'type': 'delete',
          }], [diff.get_thin_dict(d) for d in diffs])
 
@@ -62,7 +62,7 @@ def test_is_image_diff():
     assert     diff.is_image_diff(TinyDiff('foo.png', 'bar.png'))
     assert not diff.is_image_diff(TinyDiff('foo.png.gz', 'bar.png.gz'))
     assert not diff.is_image_diff(TinyDiff('foo.txt', 'bar.txt'))
-    assert     diff.is_image_diff(TinyDiff('foo.png', None))
-    assert not diff.is_image_diff(TinyDiff('foo.txt', None))
-    assert     diff.is_image_diff(TinyDiff(None, 'foo.png'))
-    assert not diff.is_image_diff(TinyDiff(None, 'foo.txt'))
+    assert     diff.is_image_diff(TinyDiff('foo.png', ''))
+    assert not diff.is_image_diff(TinyDiff('foo.txt', ''))
+    assert     diff.is_image_diff(TinyDiff('', 'foo.png'))
+    assert not diff.is_image_diff(TinyDiff('', 'foo.txt'))

webdiff/app.py

@@ -4,6 +4,8 @@
 For usage, see README.md.
 '''
 
+from __future__ import print_function
+from binaryornot.check import is_binary
 import logging
 import mimetypes
 import os
@@ -17,9 +19,9 @@
 from flask import (Flask, render_template, send_from_directory, send_file,
                    request, jsonify, Response)
 
-import diff
-import util
-import argparser
+from webdiff import diff
+from webdiff import util
+from webdiff import argparser
 
 VERSION = '0.12.1'
 
@@ -32,8 +34,8 @@ def determine_path():
             root = os.path.realpath (root)
         return os.path.dirname (os.path.abspath (root))
     except:
-        print "I'm sorry, but something is wrong."
-        print "There is no __file__ variable. Please contact the author."
+        print("I'm sorry, but something is wrong.")
+        print("There is no __file__ variable. Please contact the author.")
         sys.exit()
 
 def is_hot_reload():
@@ -101,8 +103,7 @@ def get_contents(side):
     abs_path = d.a_path if side == 'a' else d.b_path
 
     try:
-        is_binary = util.is_binary_file(abs_path)
-        if is_binary:
+        if is_binary(abs_path):
             size = os.path.getsize(abs_path)
             contents = "Binary file (%d bytes)" % size
         else:
@@ -134,7 +135,7 @@ def get_image(side, path):
     abs_path = d.a_path if side == 'a' else d.b_path
 
     try:
-        contents = open(abs_path).read()
+        contents = open(abs_path, mode='rb').read()
         return Response(contents, mimetype=mime_type)
     except Exception:
         return error('read-error', 'Unable to read %s' % abs_path)
@@ -208,6 +209,7 @@ def seriouslykill():
 
 @app.route('/kill', methods=['POST'])
 def kill():
+    global PORT
     if 'STAY_RUNNING' in app.config:
         return 'Will stay running.'
 
@@ -251,7 +253,7 @@ def pick_a_port(args):
     return port
 
 
-def abs_path(path):
+def abs_path_from_rel(path):
     '''Changes relative paths to be abs w/r/t/ the original cwd.'''
     if os.path.isabs(path):
         return path
@@ -261,7 +263,7 @@ def abs_path(path):
 
 def is_webdiff_from_head():
     '''Was webdiff invoked as `git webdiff` with no other non-flag args?'''
-    return os.environ.get('WEBDIFF_FROM_HEAD') != None
+    return os.environ.get('WEBDIFF_FROM_HEAD') is not None
 
 
 def run():

webdiff/argparser.py

@@ -3,10 +3,10 @@
 import os
 import re
 
-import dirdiff
-import githubdiff
-import github_fetcher
-from localfilediff import LocalFileDiff
+from webdiff import dirdiff
+from webdiff import githubdiff
+from webdiff import github_fetcher
+from webdiff.localfilediff import LocalFileDiff
 
 class UsageError(Exception):
     pass
@@ -20,7 +20,7 @@ class UsageError(Exception):
 ''')
 
 # e.g. https://github.com/danvk/dygraphs/pull/292
-PULL_REQUEST_RE = re.compile(r'http[s]://(?:www.)?github.com\/([^/]+)/([^/]+)/pull/([0-9]+)(?:/.*)?')
+PULL_REQUEST_RE = re.compile(r'http[s]://(?:www.)?github.com/([^/]+)/([^/]+)/pull/([0-9]+)(?:/.*)?')
 PULL_REQUEST_NUM_RE = re.compile(r'^#([0-9]+)$')
 
 def parse(args, version=None):

webdiff/diff.py

@@ -13,7 +13,7 @@
 import mimetypes
 import os
 
-import util
+from webdiff import util
 
 def get_thin_dict(diff):
     '''Returns a dict containing minimal data on the diff.
@@ -55,7 +55,7 @@ def get_thin_list(diffs, thick_idx=None):
     '''Convert a list of diffs to dicts. This adds an 'idx' field.'''
     ds = [get_thin_dict(d) for d in diffs]
     if thick_idx is not None:
-        ds[thick_idx] = get_thick_dict(ds[idx])
+        ds[thick_idx] = get_thick_dict(ds[thick_idx])
     for i, d in enumerate(ds):
         d['idx'] = i
     return ds
@@ -73,7 +73,8 @@ def is_image_diff(diff):
     This uses the a_path and b_path properties of the diff object.
     '''
     def is_image(path):
-        if path is None: return False
+        if path == '':
+            return False
         mime_type, enc = mimetypes.guess_type(path)
         return (mime_type and mime_type.startswith('image/') and enc is None)
 
@@ -82,9 +83,9 @@ def is_image(path):
 
     if left_img and right_img:
         return True
-    elif left_img and diff.b_path is None:
+    elif left_img and diff.b_path == '':
         return True
-    elif right_img and diff.a_path is None:
+    elif right_img and diff.a_path == '':
         return True
     return False
 
@@ -96,7 +97,8 @@ def find_diff_index(diffs, side, path):
     '''
     assert side in ('a', 'b')
     def norm(p):
-        if p is None: return None
+        if p == '':
+            return ''
         return os.path.normpath(p)
 
     path = norm(path)

webdiff/dirdiff.py

@@ -4,8 +4,8 @@
 import copy
 import os
 
-from localfilediff import LocalFileDiff
-import util
+from webdiff.localfilediff import LocalFileDiff
+from webdiff import util
 
 
 def diff(a_dir, b_dir):
@@ -17,7 +17,7 @@ def diff(a_dir, b_dir):
         [LocalFileDiff(a_dir, a, b_dir, b, True) for a, b in moves])
 
     # sort "change" before "delete" in a move, which is easier to understand.
-    diffs.sort(key=lambda diff: (diff.a_path, 0 if diff.b else 1))
+    diffs.sort(key=lambda d: (d.a_path, 0 if d.b else 1))
 
     return diffs
 
@@ -27,27 +27,26 @@ def find_diff(a, b):
     
     Returns a list of pairs of full paths to matched a/b files.
     '''
-    a_files = []
-    b_files = []
-    def accum(arg, dirname, fnames):
-        for fname in fnames:
-            path = os.path.join(dirname, fname)
-            if not os.path.isdir(path):
-                arg.append(path)
+
+    def list_files(top_dir):
+        file_list = []
+        for root, _, files in os.walk(top_dir):
+            root = os.path.relpath(root, start=top_dir)
+            for name in files:
+                file_list.append(os.path.join(root, name))
+        return file_list
 
     assert os.path.isdir(a)
     assert os.path.isdir(b)
 
-    os.path.walk(a, accum, a_files)
-    os.path.walk(b, accum, b_files)
-
-    a_files = [os.path.relpath(x, start=a) for x in a_files]
-    b_files = [os.path.relpath(x, start=b) for x in b_files]
+    a_files = list_files(a)
+    b_files = list_files(b)
 
     pairs = pair_files(a_files, b_files)
 
     def safejoin(d, p):
-        if p is None: return None
+        if p == '':
+            return ''
         return os.path.join(d, p)
 
     return [(safejoin(a, arel),
@@ -65,10 +64,10 @@ def pair_files(a_files, b_files):
             del a_files[i]
             del b_files[j]
         else:
-            pairs.append((f, None))  # delete
+            pairs.append((f, ''))  # delete
 
     for f in b_files:
-        pairs.append((None, f))  # add
+        pairs.append(('', f))  # add
 
     return pairs
 

webdiff/github_fetcher.py

@@ -10,10 +10,11 @@
 import os
 import re
 import subprocess
+import sys
 
 from github import Github, UnknownObjectException
 
-from util import memoize
+from webdiff.util import memoize
 
 
 @memoize
@@ -84,7 +85,7 @@ def get_pr_repo(num):
         owner = remote['owner']
         repo = remote['repo']
         try:
-            pr = g.get_user(owner).get_repo(repo).get_pull(num)
+            g.get_user(owner).get_repo(repo).get_pull(num)
             return (owner, repo, num)
         except UnknownObjectException:
             pass
@@ -133,6 +134,6 @@ def _parse_remotes(remote_lines):
 
 def _get_remotes():
     remote_lines = subprocess.Popen(
-        ['git', 'remote', '-v'], stdout=subprocess.PIPE).communicate()[0].split('\n')
+        ['git', 'remote', '-v'], stdout=subprocess.PIPE).communicate()[0].split(b'\n')
     return _parse_remotes(remote_lines)
 

webdiff/githubdiff.py

@@ -9,8 +9,8 @@
 import tempfile
 import sys
 
-from util import memoize
-from github_fetcher import github
+from webdiff.util import memoize
+from webdiff.github_fetcher import github
 
 
 class GitHubDiff(object):
@@ -25,22 +25,22 @@ def __init__(self, pr, github_file):
                 'added': 'add',
                 'removed': 'delete'
                 }[github_file.status]
-        self._a_path = None
-        self._b_path = None
+        self._a_path = ''
+        self._b_path = ''
 
     @property
     def a(self):
         if self.type == 'move':
             return self._file.raw_data['previous_filename']
         elif self.type == 'add':
-            return None
+            return ''
         else:
             return self._file.filename
 
     @property
     def b(self):
         if self.type == 'delete':
-            return None
+            return ''
         else:
             return self._file.filename
 
@@ -72,7 +72,8 @@ def fetch_pull_request(owner, repo, num):
 
 @memoize
 def fetch(repo, filename, sha):
-    if filename is None: return None
+    if filename == '':
+        return ''
     data = repo.get_file_contents(filename, sha).decoded_content
     _, ext = os.path.splitext(filename)
     fd, path = tempfile.mkstemp(suffix=ext)

webdiff/localfilediff.py

@@ -1,20 +1,19 @@
 '''This class represents the diff between two files on local disk.'''
 
 import os
-import mimetypes
-import util
+
 
 class LocalFileDiff(object):
     def __init__(self, a_root, a_path, b_root, b_path, is_move):
         '''A before/after file pair on local disk
         
         Args:
             a_path, b_path: full paths to the files on disk. Either (but not
-                both) may be None.
+                both) may be empty.
             a_root, b_root: Paths to the root of diff.
             is_move: Is this a pure move between the two files?
         '''
-        assert (a_path is not None) or (b_path is not None)
+        assert (a_path != '') or (b_path != '')
         self.a_path = a_path
         self.b_path = b_path
         self.a_root = a_root
@@ -23,19 +22,21 @@ def __init__(self, a_root, a_path, b_root, b_path, is_move):
 
     @property
     def a(self):
-        if self.a_path is None: return None
+        if self.a_path == '':
+            return ''
         return os.path.relpath(self.a_path, self.a_root)
 
     @property
     def b(self):
-        if self.b_path is None: return None
+        if self.b_path == '':
+            return ''
         return os.path.relpath(self.b_path, self.b_root)
 
     @property
     def type(self):
-        if self.a_path is None:
+        if self.a_path == '':
             return 'add'
-        elif self.b_path is None:
+        elif self.b_path == '':
             return 'delete'
         elif self.is_move:
             return 'move'