[Findit] Plain objects to represent the returned result from running the algorithm,

tools/findit/match_set.py

+# Copyright (c) 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import logging
+import re
+from threading import Lock

[aarya, 2014/08/14 21:14:39]

blank line before this line.

[jeun, 2014/08/14 22:16:48]

Done.

+
+import crash_utils
+
+
+class Match(object):
+  """Represents a match entry.
+
+  A match is a CL that is suspected to have caused the crash. A match object
+  contains information about files it changes, their authors, etc.
+
+  Attributes:
+    line_of_crash: The list of lines that caused crash for this CL.

[aarya, 2014/08/14 20:22:21]

s/line_of_crash/crash_lines

[jeun, 2014/08/14 22:16:48]

Done.

+    function: The list of functions that caused the crash.

[aarya, 2014/08/14 20:22:21]

s/function/function_list

[jeun, 2014/08/14 22:16:49]

Done.

+    min_distance: The minimum difference between the lines that CL changed and

[aarya, 2014/08/14 21:14:40]

s/difference/distance.

[jeun, 2014/08/14 22:16:48]

Done.

+                  lines that caused the crash.
+    files: The list of files that the CL changed.

[aarya, 2014/08/14 21:14:40]

s/files/changed_files

[jeun, 2014/08/14 22:16:48]

Done.

+    file_urls: The list of URLs for the file.

[aarya, 2014/08/14 21:14:40]

s/file_urls/changed_files_urls

[jeun, 2014/08/14 22:16:49]

Done.

+    author: The author of the CL.
+    component_name: The name of the component that this CL belongs to.
+    stack_frame_indices: For files that caused crash, list of where in the
+                         stackframe they occur.
+    rank: The highest priority among the files the CL changes.

[aarya, 2014/08/14 21:14:40]

Explain meaning of priorities. Priority = 0 means what, 1 means what, etc.

[jeun, 2014/08/14 22:16:48]

Done.

+    priorities: For each files, whether it changes the crashed line

[aarya, 2014/08/14 21:14:39]

s/For each files/For each file

[jeun, 2014/08/14 22:16:48]

Done.

+                (priority = 1) or is a simple file change (priority = 2).
+    reivision_url: The revision URL of the CL.
+    review_url: The codereview URL that reviews this CL.
+    reviewers: The list of people that reviewed this CL.
+    reason: The reason why this CL is suspected.
+  """
+  REVERT_PATTERN = re.compile(r'(revert\w*) r?(\d+)', re.I)
+
+  def __init__(self, revision, component_name):
+    self.is_reverted = False

[aarya, 2014/08/14 21:14:40]

put these two lines in descriptions. is_reverted and revert_of is not explained
above.

[jeun, 2014/08/14 22:16:48]

Done.

+    self.revert_of = None
+    self.line_of_crash = []
+    self.function = []
+    self.min_distance = crash_utils.INFINITY
+    self.files = []
+    self.file_urls = []
+    self.author = revision['author']
+    self.component_name = component_name
+    self.stack_frame_indices = []
+    self.rank = crash_utils.INFINITY
+    self.priorities = []
+    self.revision_url = revision['url']
+    self.review_url = 'N/A'
+    self.reviewers = ['N/A']

[aarya, 2014/08/14 21:14:40]

Why N/A and not empty list. also same for review url, why not '', instead of N/A

[jeun, 2014/08/14 22:16:48]

Done.

+    self.reason = None
+
+  def ParseMessage(self, message, codereview_api_url):
+    """Parses the message.
+
+    It checks the message to extract the code review website and list of
+    reviewers, and it also checks if the CL is a revert of another CL.
+
+    Args:
+      message: The message to parse.
+      codereview_api_url: URL to retrieve codereview data from.
+    """
+    for line in message.splitlines():
+      line = line.strip()
+
+      # Check if the line has the code review information.
+      if line.startswith('Review URL: '):

[aarya, 2014/08/14 20:22:21]

Put 'Review URL: ' in a global var and use it in both places.

[jeun, 2014/08/14 22:16:48]

Done.

+
+        # Get review number for the code review site from the line.
+        parts = line.split('Review URL: ')
+        self.review_url = parts[1].strip()
+        issue_number = self.review_url.split('/')[-1]
+
+        # Get JSON from the code review site, ignore the line if it fails.
+        url = codereview_api_url % issue_number
+        json_string = crash_utils.GetDataFromURL(url)
+        if not json_string:
+          logging.warning('Failed to retrieve code review information from %s',
+                          url)
+          continue
+
+        # Load the JSON from the string, and get the list of reviewers.
+        code_review = crash_utils.LoadJSON(json_string)
+        if code_review:
+          self.reviewers = code_review['reviewers']
+
+      # Check if this CL is a revert of other CL.
+      if line.lower().startswith('revert'):
+        self.is_reverted = True
+
+        # Check if the line says what CL this CL is a revert of.
+        revert = self.REVERT_PATTERN.match(line)
+        if revert:
+          self.revert_of = revert.group(2)
+        return
+
+
+class MatchSet(object):
+  """Represents a set of matches.
+
+  Attributes:
+    matches: A map from CL to a match object.
+    cls_to_ignore: A set of CLs to ignore.
+    matches_lock: A lock guarding matches dictionary.
+  """
+
+  def __init__(self, codereview_api_url):
+    self.codereview_api_url = codereview_api_url
+    self.matches = {}
+    self.cls_to_ignore = set()
+    self.matches_lock = Lock()
+
+  def RemoveReverts(self):

[aarya, 2014/08/14 20:22:21]

s/RemoveReverts/RemoveRevertedCLs

[jeun, 2014/08/14 22:16:48]

Done.

+    """Removes CLs that are revert."""
+    for cl in self.matches:
+      if cl in self.cls_to_ignore:
+        del self.matches[cl]