[Findit] Factoring the gitiles (etc) stuff out into its own directory

appengine/findit/common/repository.py

-# Copyright 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.
-
-
-# pylint: disable=E0711, W0613, R0201
-class Repository(object):  # pragma: no cover

[stgao, 2016/10/13 05:47:32]

I'd keep this interface. Sharu is working on an implementation of it for a local
git repo checkout. The existing git_repository.py is an implementation for the
Gitiles server. Both should conform to this interface.

[wrengr, 2016/10/14 18:33:14]

In that case I'd call the base class GitRepository (since they're both forms of
git; and hopefully they'll share a bunch of code, rather than throwing
NotImplemented everywhere), and call the two subclasses something like
LocalGitRepository and GitilesRepository.

N.B., because Python does duck typing, it's not technically necessary to have a
class hierarchy in many places; just having the right set of methods is
sufficient. This is part of why I'm advocating for using __call__ etc in various
places (since callable is a standard built-in interface, and so helps make it
easier to create one-off classes (e.g., using lambdas for mocking whatever
callable object)). Of course, the class hierarchy does help to document intent,
which is always good. It's just that, ime, doing Java-style
interfaces-as-specification where nothing is implemented tends not to work very
well for Python; for Python, class hierarchies work best when there's some
shared code.

[stgao, 2016/10/17 17:31:43]

I love the new names GitRepository, GitilesRepository, and LocalGitRepository.
Let's go with them.
But I don't anticipate too much code sharing expect parsing CL description for
metadata like commit position, codereview url, etc. That could (and should) be
factored out to be shared, because that part is actually not the responsibility
of a git repo.
I agree that a base class like this can't enforce a sublcass to always implement
all the expected methods.

However, this helps a lot to document the APIs, and leave the implementation
details to subclasses:
1. Users of those subclasses don't have to read code of the implementation or
search for public methods.
2. Contributors could refer to this base class for a new  implementation. They
don't have to deal with an existing implementation for which methods are needed
and which are not, and we could avoid missing implementation of some functions.
(e.g, this could help Sharu work on the implementation for a local git
checkout.)

[stgao, 2016/10/17 17:48:15]

Regarding __call__, I like this good idea if the class itself has only one
functionality and it is self-explained by the class name. For a class like this
to deal with Git repo, it seems not feasible or hard for readability.

OTOH, I rarely saw usage of __call__ in the infra appengine codebase, although
I'm not against its appropriate usage :)

/path/to/infra/appengine$ find . -name "*.py" | grep -v "third_party" | xargs
grep "__call__"
./chromium_rietveld/upload.py:  def __call__(self):
./chromium_status/tools/update.py:  def __call__(self, parser, namespace,
values, option_string=None):
./findit/crash/findit_for_chromecrash.py:  # be better IMO to rename it to
__call__ to reduce verbosity.
./findit/crash/scorers/scorer.py:  def __call__(self, result):
./findit/crash/scorers/aggregators.py:  def __call__(self, data_list):
./monorail/framework/framework_helpers.py:  def __call__(self):
./monorail/tools/spam/spam.py:    def __call__(self, fn):

[wrengr, 2016/10/27 17:28:00]

Done.
Ack.
Ack.

-  """An interface for source code repository."""
-
-  def GetChangeLog(self, revision):
-    """Returns the change log of the given revision."""
-    raise NotImplemented()
-
-  def GetChangeLogs(self, start_revision, end_revision, n=1000):
-    """Returns change log list in (start_revision, end_revision]."""
-    raise NotImplemented()
-
-  def GetChangeDiff(self, revision):
-    """Returns the diff of the given revision."""
-    raise NotImplemented()
-
-  def GetBlame(self, path, revision):
-    """Returns blame of the file at ``path`` of the given revision."""
-    raise NotImplemented()
-
-  def GetSource(self, path, revision):
-    """Returns source code of the file at ``path`` of the given revision."""
-    raise NotImplemented()