Issue 5336 - Allow additional include, page, and template paths using CMS

cms/sources.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 314 matching lines @@
 
     def __exit__(self, exc_type, exc_value, tb):
         return any(base.__exit__(exc_type, exc_value, tb)
                    for base in self._bases)
 
     def close(self):
         for base in self._bases:
             base.close()
 
     def has_file(self, filename):
-        for base in self._bases:

[Sebastian Noack, 2017/09/27 02:47:53]

Nit: You could use any() here as well:

  return any(base.has_file(filename) for base in self._bases)

[Vasily Kuznetsov, 2017/09/27 11:34:39]

Done.

-            if base.has_file(filename):
-                return True
-        return False
+        return any(base.has_file(filename) for base in self._bases)
 
     def read_file(self, filename, binary=False):
         for base in self._bases:
             if base.has_file(filename):
                 return base.read_file(filename, binary)
         raise KeyError('File not found {}'.format(filename))
 
     def list_files(self, subdir):
-        files = set()
-        for base in self._bases:
-            files.update(base.list_files(subdir))
-        return sorted(files)

[Sebastian Noack, 2017/09/27 02:47:53]

Nit: This could be written as a set literal one-liner:

  return {f for base in self._bases for f in base.list_files(subdir)}

Any reason why you sort? It seems the other sources also yield results in more
or less arbitrary order.

[Vasily Kuznetsov, 2017/09/27 11:34:39]

Done.

Thanks for this suggestion. Having thought about it again, there's no
particularly good reason to sort. Sorting makes files in the same subdirectory
appear together, even if they are coming from different base sources, but this
doesn't seem to be used anywhere, so it's not really important.

What might be important is the type of the return value. Sorted returns a list
while your proposal returns a set. I wrapped the set in a list for now, to be on
the safe side. Let me know if you think this is not necessary.

[Sebastian Noack, 2017/09/27 15:55:22]

The type returned here doesn't seem to be important either as FileSource gives
you a list, while MercurialSource gives you a generator. So I would just return
the set here.

[Vasily Kuznetsov, 2017/09/28 09:08:17]

Fair enough. Done.

+        return {f for base in self._bases for f in base.list_files(subdir)}
 
 
 def _memoize(func):
     """Cache results of functions calls."""
     memoized = {}
 
     def wrapper(*args):
         try:
             return memoized[args]
         except KeyError:
             return memoized.setdefault(args, func(*args))
     wrapper.cache_clear = memoized.clear
     return wrapper
 
 
-def create_source(path, static=False, revision='default'):

[Sebastian Noack, 2017/09/27 02:47:53]

I would rather implement this in Source.__new__, so that the calling code would
basically remain the same, i.e:

  FileSource(args.path)
  MercurialSource(repo, revision)

vs:

  create_source(args.path)
  create_source(repo, static=True, revision=revision)

This not only preserves a stable API (which probably isn't too important), but
that way you also don't need to introduce a boolean flag which you then map to
hard-code types, but you can just delegate to cls (first arg of __new__).

[Vasily Kuznetsov, 2017/09/27 11:34:38]

Here I prefer my implementation for the following reasons:

1. Overriding __new__ is a somewhat advanced technique that should not be used
without a good reason. I don't think we have a good reason in this case.

2. Constructors of `FileSource` and `MercurialSource` would end up sometimes
returning an object of a different class from the type the client is trying to
construct (e.g. `MultiSource` instead of `MercurialSource`). This is not the end
of the world, but confusing and better avoided.

3. The design of how `generate_static_files.py` and `test_server.py` are
constructing the source could be improved and I'm using the opportunity to start
doing it. Ideally, the code that instantiates and configures the sources would
be the same in dynamic and static cases: this ensures consistency and makes it
easier to change and test it. There was not much code in the dynamic case, only
`FileSource` instantiation, but with the change, we now make it clear that what
gets created is not necessarily a `FileSource`, it's just some subclass of
`Source`. Expectations are more clear = good! In the static case there was
caching code, that I'm not quite comfortable with because it messes too much
with the class internals -- now it's moved to `create_source` function, closer
to the source classes so the dependency is more obvious. It could be further
improved by introducing a `CachingSource`: a proxy source that completely
encapsulates the caching behavior, but let's hold our horses for now and just be
content that it's become a trivial change inside `sources.py`.

4. `static` flag is a meaningful part of the API. It tells the source
constructing machinery that we're going to be doing a one-off static generation
and not serving pages on demand, where the changes on the disk should be
observed. Right now it has the effect of enabling caching in `create_source` and
choosing `MercurialSource` instead of `FileSource` (which I'm not too happy
about, but for now we keep the behavior) but it might become something else
later. The meaning of `static` parameter is that the client is not interested in
tracking changes on the disk after the source is created and how that's ensured
is the internal business of `create_source`.

+def create_source(path, cached=False, revision=None):
     """Create a source from path and optional revision.
 
-    `static` flag determines the type of the source that will be created and
-    its caching behavior:
-    - If `static` is `True`, we use version-control-aware `MercurialSource`,
-      pass the revision and cache the results of most used functions.
-    - If `static` is `False` (as is the case with the preview server), we use
-      simpler `FileSource` and don't cache anything to be able to respond to
-      changes on the filesystem.
+    `cached` flag activates caching. This can be used to optimize performance
+    if no changes are expected on the filesystem after the source was created.
+    This is usually the case with static generation (as opposed to dynamic
+    preview).
+
+    If `revision` option is provided, the `path` is assumed to be pointing to a
+    Mercurial repository. In this case the source will return the content of
+    selected revision (using `MercurialSource`) instead of the content of the
+    directory. Note that any local changes will be ignored in this case.
 
     If `settings.ini` in the source contains `[paths]` section with an
     `additional-paths` key that contains the list of additional root folders,
     `MultiSource` will be instantiated and its bases will be the original
     source plus an additional source for each additional root folder.
     `MultiSource` looks up files in its base sources in the order they are
     provided, so the files in the additional folders will only be used if the
     original source doesn't contain that file.
     """
-    if static:
+    if revision is not None:
         source = MercurialSource(path, revision)
     else:
         source = FileSource(path)
 
     config = source.read_config()
     try:
         ap = config.get('paths', 'additional-paths').strip()
         additional_paths = filter(None, ap.split())
-    except ConfigParser.NoSectionError:
+    except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
         additional_paths = []
 
     if additional_paths:
         additional_sources = [
             create_source(os.path.join(path, p))
             for p in additional_paths
         ]
         source = MultiSource([source] + additional_sources)
 
-    if static:
+    if cached:
         for fname in [
             'resolve_link',
             'read_config',
             'read_template',
             'read_locale',
             'read_include',
             'exec_file',
         ]:
             setattr(source, fname, _memoize(getattr(source, fname)))
 
     return source