Docserver: Pull knowledge of host file systems into a single

chrome/common/extensions/docs/server2/host_file_system_provider.py

+# Copyright 2013 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.
+
+from caching_file_system import CachingFileSystem
+from local_file_system import LocalFileSystem
+from offline_file_system import OfflineFileSystem
+from subversion_file_system import SubversionFileSystem
+from third_party.json_schema_compiler.memoize import memoize
+

[not at google - send to devlin, 2013/10/08 15:48:39]

no amount of --similarity=? will convince git cl that this is a move.

[Jeffrey Yasskin, 2013/10/08 17:46:05]

Half the time I try to increase the --similarity argument, but it's actually
lowering it that tries to find more moves. I've used --similarity=10 before, and
you might have to go lower if you changed a bunch of lines in addition to moving
it.

[not at google - send to devlin, 2013/10/08 18:27:37]

hm ok I'll give it a go.

+
+class HostFileSystemProvider(object):

[Jeffrey Yasskin, 2013/10/08 17:46:05]

What's the difference between a "Creator" and a "Provider"? The memoization?

[not at google - send to devlin, 2013/10/08 18:27:37]

the memoization is an implementation detail, though it does mean that "Creator"
is obviously wrong.

It's the job of this class to provide "host" file systems to callers in some
configuration. It makes sense for those to be memoized so that the in-memory
caches don't get reset each time. Anyway, provide, hence "provider". I think
it's a very common OO term.

I updated the comment here.

+  '''Provides cached instances of host file systems for trunk or any branch.
+  '''
+  def __init__(self,
+               object_store_creator,
+               max_trunk_revision=None,
+               default_trunk_instance=None,
+               offline=False,
+               constructor_for_test=None):
+    '''
+    |object_store_creator|
+      Provides caches for file systems that need one.
+    |max_trunk_revision|
+      If not None, the maximum revision that a 'trunk' file system will be
+      created at. If None, 'trunk' file systems will use HEAD.
+    |default_trunk_instance|
+      If not None, 'trunk' file systems provided by this class without a
+      specific revision will return |default_trunk_instance| instead.
+    |offline|
+      If True all provided file systems will be wrapped in an OfflineFileSystem.
+    |constructor_for_test|
+      Provides a custom constructor rather than creating SubversionFileSystems.
+    '''
+    self._object_store_creator = object_store_creator
+    self._max_trunk_revision = max_trunk_revision
+    self._default_trunk_instance = default_trunk_instance
+    self._offline = offline
+    self._constructor_for_test = constructor_for_test

[Jeffrey Yasskin, 2013/10/08 17:46:05]

Don't do it in this change (to keep the diff small and help git notice the
move), but if you use:

  self._constructor = SubversionFileSystem.Create
  if constructor_for_test is not None:
    self._constructor = constructor_for_test

you can avoid the 'if' in _Create and make the default clearer to readers. You
can even put the default in the argument, although then you'd want to change the
name to just 'constructor' or 'filesystem_factory'.

[not at google - send to devlin, 2013/10/08 18:27:37]

SGTM.

+
+  @memoize

[Jeffrey Yasskin, 2013/10/08 17:46:05]

Memoization is dangerous in long-lived servers when the data might change. The
effect here looks like it'll be to pin a single trunk HEAD revision for the life
of the server process. Is that what you want? If so, please comment it. If I've
mis-read the actual behavior, please comment that.

[not at google - send to devlin, 2013/10/08 18:27:37]

that's not the level that the memoization happens at. in the head case this
means it'll always provide the same file system that tracks head, but that
doesn't mean that file system doesn't change as head changes.

[Jeffrey Yasskin, 2013/10/08 20:49:39]

'k, sounds good.

+  def GetTrunk(self, revision=None):

[Jeffrey Yasskin, 2013/10/08 17:46:05]

Please describe the meaning of 'revision' on trunk.

[not at google - send to devlin, 2013/10/08 18:27:37]

Done.

+    if revision is None:
+      if self._default_trunk_instance is not None:
+        return self._default_trunk_instance
+      return self._Create('trunk', revision=self._max_trunk_revision)
+    if self._max_trunk_revision is not None:
+      revision = min(revision, self._max_trunk_revision)
+    return self._Create('trunk', revision=revision)
+
+  @memoize

[Jeffrey Yasskin, 2013/10/08 17:46:05]

Branches also change over time, but maybe we don't care about having up-to-date
data about features merged into branches?

[not at google - send to devlin, 2013/10/08 18:27:37]

correct, branches change, and it would be seriously confusing if that happened
mid-cron run.

[Jeffrey Yasskin, 2013/10/08 20:49:39]

Given my misunderstanding about this memoization that you corrected above, do we
need this function to take an optional |revision| also? (Not in this change, of
course.)

[not at google - send to devlin, 2013/10/08 21:29:36]

Strictly speaking yes. We should be making sure that no branches change
underneath us while we're reading.

Realistically, only trunk ever changes.

I will add comment.

+  def GetBranch(self, branch):
+    '''Creates either SVN file systems or specialized file systems from the

[Jeffrey Yasskin, 2013/10/08 17:46:05]

This seems like it still belongs on _Create. You should document GetBranch at a
higher level.

[not at google - send to devlin, 2013/10/08 18:27:37]

Done.

+    constructor passed into this instance. Wraps the resulting file system in
+    an Offline file system if the offline flag is set, and finally wraps it in a
+    Caching file system.
+    '''
+    assert isinstance(branch, basestring), 'Branch %s must be a string' % branch
+    assert branch != 'trunk', 'Cannot specify branch=\'trunk\', use GetTrunk()'
+    return self._Create(branch)
+
+  def _Create(self, branch, revision=None):
+    if self._constructor_for_test is not None:
+      file_system = self._constructor_for_test(branch=branch, revision=revision)
+    else:
+      file_system = SubversionFileSystem.Create(branch=branch,
+                                                revision=revision)
+    if self._offline:
+      file_system = OfflineFileSystem(file_system)
+    return CachingFileSystem(file_system, self._object_store_creator)
+
+  @staticmethod

[Jeffrey Yasskin, 2013/10/08 17:46:05]

If these had been @classmethod, you wouldn't have had to change them at all when
you renamed the class:

@classmethod
def ForLocal(cls, object_store_creator):
  return cls(object_store_creator, ...)

[not at google - send to devlin, 2013/10/08 18:27:37]

that sounds like a nice pattern in general, thanks.

+  def ForLocal(object_store_creator):
+    '''Used in creating a server instance on localhost.
+    '''
+    return HostFileSystemProvider(
+        object_store_creator,
+        constructor_for_test=lambda **_: LocalFileSystem.Create())

[Jeffrey Yasskin, 2013/10/08 17:46:05]

'constructor_for_test' seems like the wrong name if you're using it outside of
tests.

[not at google - send to devlin, 2013/10/08 18:27:37]

Local only makes sense when testing (or when running preview.py), though in this
case it's an implementation detail. Perhaps it makes sense to combine "default
trunk instance" and "constructor for test" into a more general abstraction but
it's approaching abstraction overload.

+
+  @staticmethod
+  def ForTest(file_system, object_store_creator):
+    '''Used in creating a test server instance. The HostFileSystemProvider
+    returned here will always return |file_system| when its Create() method is
+    called.
+    '''
+    return HostFileSystemProvider(
+        object_store_creator,
+        constructor_for_test=lambda **_: file_system)