Changed FindSuspects to take a Repository factory, rather than mutating it

appengine/findit/crash/changelist_classifier.py

Index: appengine/findit/crash/changelist_classifier.py
diff --git a/appengine/findit/crash/changelist_classifier.py b/appengine/findit/crash/changelist_classifier.py
index 2d84b021f296ebcafea7f8cbd01ddfafdcfde9e5..0bb80faa3cbd8adee6a573bfbeec1916443d15c5 100644
--- a/appengine/findit/crash/changelist_classifier.py
+++ b/appengine/findit/crash/changelist_classifier.py
@@ -6,7 +6,7 @@ import logging
 from collections import defaultdict
 from collections import namedtuple
 
-from common import chrome_dependency_fetcher
+from common.chrome_dependency_fetcher import ChromeDependencyFetcher
 from crash import crash_util
 from crash.suspect import StackInfo
 from crash.suspect import Suspect
@@ -20,18 +20,27 @@ from libs.gitiles.diff import ChangeType
 
 
 class ChangelistClassifier(namedtuple('ChangelistClassifier',
-    ['repository', 'top_n_results', 'confidence_threshold'])):
+    ['repository', 'get_repository', 'top_n_results', 'confidence_threshold'])):

[Sharu Jiang, 2016/12/28 00:45:33]

The ``get_repository`` here is not actually a factory, it is to mutate
repository, so this name is a little misleading. Better add comments to explain
that it will be cleaned in the future. 

[Sharu Jiang, 2016/12/28 00:45:33]

So after we cleaned the mutation in ``get_repository``(make it allocates new
repository object instead of doing mutation), we will get rid of this
``repository`` parameter, right?

[wrengr, 2016/12/28 18:23:33]

The ``get_repository`` argument is indeed (intended to be) a factory.
``FindSuspects`` doesn't rely on/require it to return novel objects (e.g., it
doesn't save them anywhere or anything requiring longevity beyond from when
``get_repository`` is called to when ``FindSuspects`` returns). Which is why we
can get away with passing a fake "factory" that doesn't actually build anything.
Yes, passing a fake factory is bad; but that's firmly the fault of the caller
not the callee.

[wrengr, 2016/12/28 18:23:33]

In principle we could... but since the main ``repository`` is used frequently
and longlived-ly, we should probably keep it. This way the caller of
ChangelistClassifier.__init__ can build the repo object once, pass it to the
constructor, and then forget about it, the url associated with it, etc (if
needed, the caller can always retrieve that information from the
ChangelistClassifier object itself).

The simplest alternative to keeping it around would be to pass the url to the
constructor, and understand that the constructor will call ``get_repository`` on
it and save the result. That seems less clean than passing the repo object, imo

   __slots__ = ()
 
-  def __new__(cls, repository, top_n_results=3, confidence_threshold=0.999):
+  def __new__(cls, repository, get_repository, top_n_results=3,
+              confidence_threshold=0.999):
     """Args:
-      repository (Repository): the Git repository for getting CLs to classify.
+      repository (Repository): the Git repository of the main project
+        we're trying to classify CLs from.
+      get_repository (callable): a function from DEP urls to ``Repository``
+        objects, so we can get changelogs and blame for each dep. Notably,
+        to keep the code here generic, we make no assumptions about
+        which subclass of ``Repository`` this function returns. Thus,
+        it is up to the caller to decide what class to return and handle
+        any other arguments that class may require (e.g., an http client
+        for ``GitilesRepository``).
       top_n_results (int): maximum number of results to return.
       confidence_threshold (float): In [0,1], above which we only return
         the first suspect.
     """
     return super(cls, ChangelistClassifier).__new__(
-        cls, repository, top_n_results, confidence_threshold)
+        cls, repository, get_repository, top_n_results, confidence_threshold)
 
   def __str__(self): # pragma: no cover
     return ('%s(top_n_results=%d, confidence_threshold=%g)'
@@ -59,6 +68,8 @@ class ChangelistClassifier(namedtuple('ChangelistClassifier',
     logging.info('ChangelistClassifier.__call__: Regression range %s:%s',
         last_good_version, first_bad_version)
 
+    dependency_fetcher = ChromeDependencyFetcher(self.repository)

[Sharu Jiang, 2016/12/28 00:45:33]

I think ``ChromeDependencyFetcher`` should also take a repository factory as
construct parameter instead of repository instance, because it's ``Chrome``
dependency fetcher, which means the dep url has to be chromium dep url, so
repository should be created inside ``ChromeDependencyFetcher`` with dep url set
to chromium url.

[wrengr, 2016/12/28 18:23:33]

I'm not sure I follow. Perhaps you could write up a CL demonstrating the change
to ChromeDependencyFetcher you have in mind

[wrengr, 2016/12/28 19:09:24]

ah, nm. I think I see what you mean now. I'm working on cleaning u the
ChromeDependencyFetcher stuff too in the CL that gets rid of the mutation

[wrengr, 2016/12/28 20:55:52]

See https://codereview.chromium.org/2605943002 for cleaning all this up

+
     # We are only interested in the deps in crash stack (the callstack that
     # caused the crash).
     # TODO(wrengr): we may want to receive the crash deps as an argument,
@@ -66,14 +77,12 @@ class ChangelistClassifier(namedtuple('ChangelistClassifier',
     # doing redundant work creating it.
     stack_deps = GetDepsInCrashStack(
         report.stacktrace.crash_stack,
-        chrome_dependency_fetcher.ChromeDependencyFetcher(
-            self.repository).GetDependency(report.crashed_version,
-                                           report.platform))
+        dependency_fetcher.GetDependency(
+            report.crashed_version, report.platform))
 
     # Get dep and file to changelogs, stack_info and blame dicts.
-    dep_rolls = chrome_dependency_fetcher.ChromeDependencyFetcher(
-        self.repository).GetDependencyRollsDict(
-            last_good_version, first_bad_version, report.platform)
+    dep_rolls = dependency_fetcher.GetDependencyRollsDict(
+        last_good_version, first_bad_version, report.platform)
 
     # Regression of a dep added/deleted (old_revision/new_revision is None) can
     # not be known for sure and this case rarely happens, so just filter them
@@ -91,11 +100,9 @@ class ChangelistClassifier(namedtuple('ChangelistClassifier',
     dep_to_file_to_stack_infos = GetStackInfosForFilesGroupedByDeps(
         report.stacktrace, stack_deps)
 
-    # TODO: argument order is inconsistent from others. Repository should
-    # be last argument.
     suspects = FindSuspects(dep_to_file_to_changelogs,
                             dep_to_file_to_stack_infos,
-                            stack_deps, self.repository, ignore_cls)
+                            stack_deps, self.get_repository, ignore_cls)
     if not suspects:
       return []
 
@@ -257,7 +264,7 @@ def GetStackInfosForFilesGroupedByDeps(stacktrace, stack_deps):
 # TODO(katesonia): Remove the repository argument after refatoring cl committed.
 def FindSuspects(dep_to_file_to_changelogs,
                  dep_to_file_to_stack_infos,
-                 stack_deps, repository,
+                 stack_deps, get_repository,
                  ignore_cls=None):
   """Finds suspects by matching stacktrace and changelogs in regression range.
 
@@ -271,7 +278,8 @@ def FindSuspects(dep_to_file_to_changelogs,
       frames, one stack info consist of a StackFrame and the callstack priority
       of it.
     stack_deps (dict): Represents all the dependencies shown in the crash stack.
-    repository (Repository): Repository to get changelogs and blame from.
+    get_repository (callable): a function from urls to ``Repository``
+      objects, so we can get changelogs and blame for each dep.
     ignore_cls (set): Set of reverted revisions.
 
   Returns:
@@ -287,7 +295,7 @@ def FindSuspects(dep_to_file_to_changelogs,
         if not crash_util.IsSameFilePath(crashed_file_path, touched_file_path):
           continue
 
-        repository.repo_url = stack_deps[dep].repo_url
+        repository = get_repository(stack_deps[dep].repo_url)
         blame = repository.GetBlame(touched_file_path,
                                     stack_deps[dep].revision)