Merge pull request #125 from numberMumbler/issue_47

Consolidate diff parsing logic

eventhandler.py

@@ -58,8 +58,8 @@ def get_warnings():
 def get_handlers():
     modules = []
     handlers = []
-    possiblehandlers = os.listdir('handlers')
-    for i in possiblehandlers:
+    possible_handlers = os.listdir('handlers')
+    for i in possible_handlers:
         location = os.path.join('handlers', i)
         try:
             module = imp.load_module('handlers.' + i, None, location,

handlers/empty_title_element/__init__.py

@@ -1,17 +1,14 @@
 from __future__ import absolute_import
-
 from eventhandler import EventHandler
-from helpers import is_addition
 
 WARNING = ("These commits include an empty title element (`<title></title>`). "
            "Consider adding appropriate metadata.")
 
 
 class EmptyTitleElementHandler(EventHandler):
     def on_pr_opened(self, api, payload):
-        diff = api.get_diff()
-        for line in diff.split('\n'):
-            if is_addition(line) and line.find("<title></title>") > -1:
+        for line in api.get_added_lines():
+            if line.find("<title></title>") > -1:
                 # This test doesn't consider case and whitespace the same way
                 # that a HTML parser does, so empty title elements might still
                 # go unnoticed. It will catch the low-hanging fruit, though.

handlers/missing_test/__init__.py

@@ -11,18 +11,16 @@ class MissingTestHandler(EventHandler):
     TEST_DIRS_TO_CHECK = ('ref', 'wpt', 'unit')
 
     def on_pr_opened(self, api, payload):
-        diff = api.get_diff()
         components_changed = set()
 
-        for line in diff.split('\n'):
-            if line.startswith('diff --git'):
-                for component in self.COMPONENT_DIRS_TO_CHECK:
-                    if 'components/{0}/'.format(component) in line:
-                        components_changed.add(component)
+        for filepath in api.get_changed_files():
+            for component in self.COMPONENT_DIRS_TO_CHECK:
+                if 'components/{0}/'.format(component) in filepath:
+                    components_changed.add(component)
 
-                for directory in self.TEST_DIRS_TO_CHECK:
-                    if 'tests/{0}'.format(directory) in line:
-                        return
+            for directory in self.TEST_DIRS_TO_CHECK:
+                if 'tests/{0}'.format(directory) in filepath:
+                    return
 
         if components_changed:
             # Build a readable list of changed components

handlers/no_modify_css_tests/__init__.py

@@ -11,8 +11,8 @@ class NoModifyCSSTestsHandler(EventHandler):
     DIR_TO_CHECK = "tests/wpt/css-tests"
 
     def on_pr_opened(self, api, payload):
-        for line in api.get_diff().split('\n'):
-            if line.startswith("diff --git") and self.DIR_TO_CHECK in line:
+        for filepath in api.get_changed_files():
+            if self.DIR_TO_CHECK in filepath:
                 self.warn(NO_MODIFY_CSS_TESTS_MSG)
                 break
 

handlers/nonini_wpt_meta/__init__.py

@@ -19,19 +19,18 @@ class NonINIWPTMetaFileHandler(EventHandler):
     )
 
     def _wpt_ini_dirs(self, line):
-        if line.startswith('diff --git') and '.' in line \
-           and not any(fp in line for fp in self.FALSE_POSITIVE_SUBSTRINGS):
+        if '.' in line and not any(fp in line
+                                   for fp in self.FALSE_POSITIVE_SUBSTRINGS):
             return set(directory for directory in self.DIRS_TO_CHECK
                        if directory in line)
         else:
             return set()
 
     def on_pr_opened(self, api, payload):
-        diff = api.get_diff()
         test_dirs_with_offending_files = set()
 
-        for line in diff.split('\n'):
-            test_dirs_with_offending_files |= self._wpt_ini_dirs(line)
+        for filepath in api.get_changed_files():
+            test_dirs_with_offending_files |= self._wpt_ini_dirs(filepath)
 
         if test_dirs_with_offending_files:
             if len(test_dirs_with_offending_files) == 1:

handlers/unsafe/__init__.py

@@ -1,7 +1,6 @@
 from __future__ import absolute_import
 
 from eventhandler import EventHandler
-from helpers import is_addition
 
 
 unsafe_warning_msg = ('These commits modify **unsafe code**. '
@@ -10,9 +9,8 @@
 
 class UnsafeHandler(EventHandler):
     def on_pr_opened(self, api, payload):
-        diff = api.get_diff()
-        for line in diff.split('\n'):
-            if is_addition(line) and line.find('unsafe ') > -1:
+        for line in api.get_added_lines():
+            if line.find('unsafe ') > -1:
                 self.warn(unsafe_warning_msg)
                 return
 

handlers/watchers/__init__.py

@@ -20,16 +20,6 @@ def build_message(mentions):
 class WatchersHandler(EventHandler):
     def on_pr_opened(self, api, payload):
         user = payload['pull_request']['user']['login']
-        diff = api.get_diff()
-        changed_files = []
-        for line in diff.split('\n'):
-            if line.startswith('diff --git'):
-                changed_files.extend(line.split('diff --git ')[-1].split(' '))
-
-        # Remove the `a/` and `b/` parts of paths,
-        # And get unique values using `set()`
-        changed_files = set(map(lambda f: f if f.startswith('/') else f[2:],
-                                changed_files))
 
         watchers = get_people_from_config(api, WATCHERS_CONFIG_FILE)
         if not watchers:
@@ -44,15 +34,15 @@ def on_pr_opened(self, api, payload):
                     blacklisted_files.append(watched_file[1:])
             for blacklisted_file in blacklisted_files:
                 watched_files.remove('-' + blacklisted_file)
-            for changed_file in changed_files:
+            for filepath in api.get_changed_files():
                 for blacklisted_file in blacklisted_files:
-                    if changed_file.startswith(blacklisted_file):
+                    if filepath.startswith(blacklisted_file):
                         break
                 else:
                     for watched_file in watched_files:
-                        if (changed_file.startswith(watched_file) and
+                        if (filepath.startswith(watched_file) and
                                 user != watcher):
-                            mentions[watcher].append(changed_file)
+                            mentions[watcher].append(filepath)
 
         if not mentions:
             return

helpers.py

@@ -7,6 +7,9 @@
                                          'collaborators.ini')
 
 
+_test_path_roots = ['a/', 'b/']
+
+
 def get_people_from_config(api, config_abs_path):
     config = ConfigParser.ConfigParser()
     config.read(config_abs_path)
@@ -30,13 +33,27 @@ def is_addition(diff_line):
     return diff_line.startswith('+') and not diff_line.startswith('+++')
 
 
+def normalize_file_path(filepath):
+    """
+    Strip any leading/training whitespace.
+    Remove any test directories from the start of the path
+    """
+    if filepath is None or filepath.strip() == '':
+        return None
+    filepath = filepath.strip()
+    for prefix in _test_path_roots:
+        if filepath.startswith(prefix):
+            return filepath[len(prefix):]
+    return filepath
+
+
 def linear_search(sequence, element, callback=lambda thing: thing):
-    '''
+    """
     The 'in' operator also does a linear search over a sequence, but it checks
     for the exact match of the given object, whereas this makes use of '==',
     which calls the '__eq__' magic method, which could've been overridden in
     a custom class (which is the case for our test lint)
-    '''
+    """
     for thing in sequence:
         if element == thing:    # element could have an overridden '__eq__'
             callback(thing)

newpr.py

@@ -16,6 +16,9 @@
 import urllib2
 
 import eventhandler
+from helpers import is_addition, normalize_file_path
+
+DIFF_HEADER_PREFIX = 'diff --git '
 
 
 class APIProvider(object):
@@ -25,6 +28,7 @@ def __init__(self, payload, user):
         self.repo = repo
         self.issue = issue
         self.user = user
+        self.changed_files = None
 
     def is_new_contributor(self, username):
         raise NotImplementedError
@@ -53,6 +57,31 @@ def get_pull(self):
     def get_page_content(self, url):
         raise NotImplementedError
 
+    def get_diff_headers(self):
+        diff = self.get_diff()
+        for line in diff.splitlines():
+            if line.startswith(DIFF_HEADER_PREFIX):
+                yield line
+
+    def get_changed_files(self):
+        if self.changed_files is None:
+            changed_files = []
+            for line in self.get_diff_headers():
+                files = line.split(DIFF_HEADER_PREFIX)[-1].split(' ')
+                changed_files.extend(files)
+
+            # And get unique values using `set()`
+            normalized = map(normalize_file_path, changed_files)
+            self.changed_files = set(f for f in normalized if f is not None)
+        return self.changed_files
+
+    def get_added_lines(self):
+        diff = self.get_diff()
+        for line in diff.splitlines():
+            if is_addition(line):
+                # prefix of one or two pluses (+)
+                yield line
+
 
 class GithubAPIProvider(APIProvider):
     BASE_URL = "https://api.github.com/repos/"