[webkitpy] Refactor diff_parser.py

Tools/Scripts/webkitpy/common/checkout/diff_parser.py

Index: Tools/Scripts/webkitpy/common/checkout/diff_parser.py
diff --git a/Tools/Scripts/webkitpy/common/checkout/diff_parser.py b/Tools/Scripts/webkitpy/common/checkout/diff_parser.py
index 3a9ea9224f7cda0d2f81fd3db2f25c304d5f3821..c5e4373012e546dfa51f8ae23d706c58f53b54b3 100644
--- a/Tools/Scripts/webkitpy/common/checkout/diff_parser.py
+++ b/Tools/Scripts/webkitpy/common/checkout/diff_parser.py
@@ -34,63 +34,6 @@ import re
 _log = logging.getLogger(__name__)
 
 
-# FIXME: This is broken. We should compile our regexps up-front
-# instead of using a custom cache.
-_regexp_compile_cache = {}
-
-
-# FIXME: This function should be removed.
-def match(pattern, string):
-    """Matches the string with the pattern, caching the compiled regexp."""
-    if not pattern in _regexp_compile_cache:
-        _regexp_compile_cache[pattern] = re.compile(pattern)
-    return _regexp_compile_cache[pattern].match(string)
-
-
-# FIXME: This belongs on DiffParser (e.g. as to_svn_diff()).
-def git_diff_to_svn_diff(line):
-    """Converts a git formatted diff line to a svn formatted line.
-
-    Args:
-      line: A string representing a line of the diff.
-    """
-    # FIXME: This list should be a class member on DiffParser.
-    # These regexp patterns should be compiled once instead of every time.
-    conversion_patterns = (("^diff --git \w/(.+) \w/(?P<FilePath>.+)", lambda matched: "Index: " + matched.group('FilePath') + "\n"),
-                           ("^new file.*", lambda matched: "\n"),
-                           ("^index (([0-9a-f]{7}\.\.[0-9a-f]{7})|([0-9a-f]{40}\.\.[0-9a-f]{40})) [0-9]{6}", lambda matched: "===================================================================\n"),
-                           ("^--- \w/(?P<FilePath>.+)", lambda matched: "--- " + matched.group('FilePath') + "\n"),
-                           ("^\+\+\+ \w/(?P<FilePath>.+)", lambda matched: "+++ " + matched.group('FilePath') + "\n"))
-
-    for pattern, conversion in conversion_patterns:
-        matched = match(pattern, line)
-        if matched:
-            return conversion(matched)
-    return line
-
-
-# This function exists so we can unittest get_diff_converter function
-def svn_diff_to_svn_diff(line):
-    return line
-
-
-# FIXME: This method belongs on DiffParser
-def get_diff_converter(lines):
-    """Gets a converter function of diff lines.
-
-    Args:
-      lines: The lines of a diff file.
-             If this line is git formatted, we'll return a
-             converter from git to SVN.
-    """
-    for i, line in enumerate(lines[:-1]):
-        # Stop when we find the first patch
-        if line[:3] == "+++" and lines[i + 1] == "---":
-            break
-        if match(r"^diff --git \w/", line):
-            return git_diff_to_svn_diff
-    return svn_diff_to_svn_diff
-
 _INITIAL_STATE = 1
 _DECLARED_FILE_PATH = 2
 _PROCESSING_CHUNK = 3
@@ -134,6 +77,17 @@ class DiffParser(object):
     The field "files" is a dict whose key is the filename and value is
     a DiffFile object.
     """
+    conversion_patterns = (
+        (re.compile("^diff --git \w/(.+) \w/(?P<FilePath>.+)"), lambda matched: "Index: " + matched.group('FilePath') + "\n"),
+        (re.compile("^new file.*"), lambda matched: "\n"),
+        (re.compile("^index (([0-9a-f]{7}\.\.[0-9a-f]{7})|([0-9a-f]{40}\.\.[0-9a-f]{40})) [0-9]{6}"), lambda matched: ("=" * 67) + "\n"),
+        (re.compile("^--- \w/(?P<FilePath>.+)"), lambda matched: "--- " + matched.group('FilePath') + "\n"),
+        (re.compile("^\+\+\+ \w/(?P<FilePath>.+)"), lambda matched: "+++ " + matched.group('FilePath') + "\n"),
+    )
+
+    index_pattern = re.compile(r"^Index: (?P<FilePath>.+)")
+    lines_changed_pattern = re.compile(r"^@@ -(?P<OldStartLine>\d+)(,\d+)? \+(?P<NewStartLine>\d+)(,\d+)? @@")
+    diff_git_pattern = re.compile(r"^diff --git \w/")

[Dirk Pranke, 2014/10/08 01:45:47]

I'm not generally a fan of class-level constants that then get accessed by class
methods. Such things tend to make most sense when there might be subclasses that
will override these values, but that won't happen here.

We shouldn't use class-level fields as a code structuring mechanism; that's what
packages and modules are for. 

Can we move these to just be module-level constants and methods?

[elecro, 2014/10/08 08:14:16]

Yeah, we can move them, but the FIXMEs mentioned to move the methods into the
DiffParser :). So I'll update the patch and remove the fixmes also.

 
     def __init__(self, diff_input):
         """Parses a diff.
@@ -150,12 +104,12 @@ class DiffParser(object):
         current_file = None
         old_diff_line = None
         new_diff_line = None
-        transform_line = get_diff_converter(diff_input)
+        transform_line = self.get_diff_converter(diff_input)
         for line in diff_input:
             line = line.rstrip("\n")
             line = transform_line(line)
 
-            file_declaration = match(r"^Index: (?P<FilePath>.+)", line)
+            file_declaration = self.index_pattern.match(line)
             if file_declaration:
                 filename = file_declaration.group('FilePath')
                 current_file = DiffFile(filename)
@@ -163,7 +117,7 @@ class DiffParser(object):
                 state = _DECLARED_FILE_PATH
                 continue
 
-            lines_changed = match(r"^@@ -(?P<OldStartLine>\d+)(,\d+)? \+(?P<NewStartLine>\d+)(,\d+)? @@", line)
+            lines_changed = self.lines_changed_pattern.match(line)
             if lines_changed:
                 if state != _DECLARED_FILE_PATH and state != _PROCESSING_CHUNK:
                     _log.error('Unexpected line change without file path '
@@ -191,3 +145,35 @@ class DiffParser(object):
                     _log.error('Unexpected diff format when parsing a '
                                'chunk: %r' % line)
         return files
+
+    @classmethod
+    def get_diff_converter(cls, lines):
+        """Gets a converter function of diff lines.
+
+        Args:
+          lines: The lines of a diff file.
+                 If this line is git formatted, we'll return a
+                 converter from git to SVN.
+        """
+        for i, line in enumerate(lines[:-1]):
+            if cls.diff_git_pattern.match(line):
+                return cls.git_diff_to_svn_diff
+        return cls.svn_diff_to_svn_diff
+
+    @classmethod
+    def git_diff_to_svn_diff(cls, line):
+        """Converts a git formatted diff line to a svn formatted line.
+
+        Args:
+            line: A string representing a line of the diff.
+        """
+        for pattern, conversion in cls.conversion_patterns:
+            matched = pattern.match(line)
+            if matched:
+                return conversion(matched)
+        return line
+
+    # This function exists so we can unittest get_diff_converter function
+    @classmethod
+    def svn_diff_to_svn_diff(cls, line):
+        return line