Implement JST/Handlebars as concat() filters.

This is a much better approach, no longer requires abusing multiple filter
functions to work in concert, fixes the caching issue of miracle2k#188, and in general
should save us some headache.

As part of this, FilterTool.apply_func was changed to raise an exception if no
filters run. This allows us to differentiate between no filter running and a
filter not returning anything. We didn't end up needing this here in the end,
but it is useful nonetheless.

docs/upgrading.rst

@@ -135,6 +135,14 @@ In Development version
   output as raw strings. Before, ``False`` behaved like ``None`` and used
   the builtin compiler.
 
+- The API of the ``concat()`` filter method has changed. Instead of a
+  list of hunks, it is now given a list of 2-tuples of
+  ``(hunk, info_dict)``.
+
+- The internal ``JSTTemplateFilter`` base class has changed API.
+  - concat filter
+  - jst handlebar filters have changed, use concat, base class has changed
+
 
 In 0.7
 ~~~~~~

src/webassets/bundle.py

@@ -4,7 +4,7 @@
 
 from filter import get_filter
 from merge import (FileHunk, UrlHunk, FilterTool, merge, merge_filters,
-                   select_filters, MoreThanOneFilterError)
+                   select_filters, MoreThanOneFilterError, NoFilters)
 from updater import SKIP_CACHE
 from exceptions import BundleError, BuildError
 from utils import cmp_debug_levels
@@ -343,7 +343,7 @@ def _merge_and_apply(self, env, output, force, parent_debug=None,
 
         # We construct two lists of filters. The ones we want to use in this
         # iteration, and the ones we want to pass down to child bundles.
-        # Why? Say we are in merge mode. Assume an "input()" filter  which does
+        # Why? Say we are in merge mode. Assume an "input()" filter which does
         # not run in merge mode, and a child bundle that switches to
         # debug=False. The child bundle then DOES want to run those input
         # filters, so we do need to pass them.
@@ -378,11 +378,13 @@ def _merge_and_apply(self, env, output, force, parent_debug=None,
         hunks = []
         for item, cnt in resolved_contents:
             if isinstance(cnt, Bundle):
+                # Recursively process nested bundles.
                 hunk = cnt._merge_and_apply(
                     env, output, force, current_debug_level,
                     filters_to_pass_down, disable_cache=disable_cache)
                 if hunk is not None:
-                    hunks.append(hunk)
+                    hunks.append((hunk, {}))
+
             else:
                 # Give a filter the chance to open his file.
                 try:
@@ -403,19 +405,23 @@ def _merge_and_apply(self, env, output, force, parent_debug=None,
                         cache_key=[FileHunk(cnt)] if not is_url(cnt) else [])
                 except MoreThanOneFilterError, e:
                     raise BuildError(e)
-
-                if not hunk:
+                except NoFilters:
+                    # Open the file ourselves.
                     if is_url(cnt):
                         hunk = UrlHunk(cnt, env=env)
                     else:
                         hunk = FileHunk(cnt)
 
-                hunks.append(filtertool.apply(
-                    hunk, filters_to_run, 'input',
-                    # Pass along both the original relative path, as
-                    # specified by the user, and the one that has been
-                    # resolved to a filesystem location.
-                    kwargs={'source': item, 'source_path': cnt}))
+                # With the hunk, remember both the original relative
+                # path, as specified by the user, and the one that has
+                # been resolved to a filesystem location. We'll pass
+                # them along to various filter steps.
+                item_data = {'source': item, 'source_path': cnt}
+
+                # Run input filters, unless open() told us not to.
+                hunk = filtertool.apply(hunk, filters_to_run, 'input',
+                                            kwargs=item_data)
+                hunks.append((hunk, item_data))
 
         # If this bundle is empty (if it has nested bundles, they did
         # not yield any hunks either), return None to indicate so.
@@ -426,10 +432,10 @@ def _merge_and_apply(self, env, output, force, parent_debug=None,
         # a filter here, by implementing a concat() method.
         try:
             final = filtertool.apply_func(filters_to_run, 'concat', [hunks])
-            if final is None:
-                final = merge(hunks)
-        except (IOError, MoreThanOneFilterError), e:
+        except MoreThanOneFilterError, e:
             raise BuildError(e)
+        except NoFilters:
+            final = merge([h for h, _ in hunks])
 
         # Apply output filters.
         # TODO: So far, all the situations where bundle dependencies are

src/webassets/filter/__init__.py

@@ -285,7 +285,7 @@ def concat(self, out, hunks, **kw):
 
        Will be called once between the input() and output()
        steps, and should concat all the source files (given as hunks)
-       together, and return a string.
+       together, writing the result to the ``out`` stream.
 
        Only one such filter is allowed.
        """

src/webassets/filter/handlebars.py

@@ -3,7 +3,6 @@
 
 from webassets.exceptions import FilterError
 from webassets.filter.jst import JSTemplateFilter
-from webassets.merge import FileHunk
 
 
 __all__ = ('Handlebars',)
@@ -15,11 +14,27 @@ class Handlebars(JSTemplateFilter):
     This filter assumes that the ``handlebars`` executable is in the path.
     Otherwise, you may define a ``HANDLEBARS_BIN`` setting.
 
-    Note: Use this filter if you want to precompile Handlebars templates.
-    If compiling them in the browser is acceptable, you may use the JST
-    filter, which needs no external dependency.
+    .. note::
+        Use this filter if you want to precompile Handlebars templates.
+        If compiling them in the browser is acceptable, you may use the
+        JST filter, which needs no external dependency.
+
+    .. warning::
+        Currently, this filter is not compatible with input filters. Any
+        filters that would run during the input-stage will simply be
+        ignored. Input filters tend to be other compiler-style filters,
+        so this is unlikely to be an issue.
     """
 
+    # TODO: We should fix the warning above. Either, me make this filter
+    # support input-processing (we'd have to work with the hunks given to
+    # us, rather than the original source files), or make webassets raise
+    # an error if the handlebars filter is combined with an input filter.
+    # I'm unsure about the best API design. We could support open()
+    # returning ``True`` to indicate "no input filters allowed" (
+    # surprisingly hard to implement) Or, use an attribute to declare
+    # as much.
+
     name = 'handlebars'
     options = {
         'binary': 'HANDLEBARS_BIN',
@@ -28,28 +43,22 @@ class Handlebars(JSTemplateFilter):
     }
     max_debug_level = None
 
-    # XXX Due to the way this filter works, any other filters applied
-    # WILL BE IGNORED. Maybe this method should be allowed to return True
-    # to indicate that the input() processor is not supported.
-    def open(self, out, source_path, **kw):
-        self.templates.append(source_path)
-        # Write back or the cache would not detect changes
-        out.write(FileHunk(source_path).data())
+    def process_templates(self, out,  hunks, **kw):
+        templates = [info['source_path'] for _, info in hunks]
 
-    def output(self, _in, out, **kw):
         if self.root is True:
             root = self.get_config('directory')
         elif self.root:
             root = path.join(self.get_config('directory'), self.root)
         else:
-            root = self._find_base_path(self.templates)
+            root = self._find_base_path(templates)
 
         args = [self.binary or 'handlebars']
         if root:
             args.extend(['-r', root])
         if self.extra_args:
             args.extend(self.extra_args)
-        args.extend(self.templates)
+        args.extend(templates)
 
         proc = subprocess.Popen(
             args, stdin=subprocess.PIPE,

src/webassets/filter/jst.py

@@ -11,10 +11,28 @@ class JSTemplateFilter(Filter):
     possibly other Javascript templating systems in the future.
     """
 
-    def setup(self):
-        super(JSTemplateFilter, self).setup()
-        # Reset template collection (same instance may run multiple times)
-        self.templates = []
+    def concat(self, out, hunks, **kwargs):
+        self.process_templates(out, hunks, **kwargs)
+
+    def process_templates(self, out, hunks, **kw):
+        raise NotImplementedError()
+
+    def iter_templates_with_base(self, hunks):
+        """Helper that for list of ``hunks``, as given to
+        ``concat()``, yields 2-tuples of (name, hunk), with name
+        being the name of the source file relative to the common
+        prefix of all source files.
+
+        In other words, each template gets the shortest possible
+        name to identify it.
+        """
+        base_path = self._find_base_path(
+            [info['source_path'] for _, info in hunks]) + os.path.sep
+        for hunk, info in hunks:
+            name = info['source_path']
+            name = name[len(base_path):]
+            name = os.path.splitext(name)[0]
+            yield name, hunk
 
     def _find_base_path(self, paths):
         """Hmmm.  There should aways be some common base path."""
@@ -118,31 +136,22 @@ def setup(self):
         self.include_jst_script = (self.template_function == 'template') \
                                   or not self.template_function
 
-    def input(self, _in, out, source_path, output_path, **kw):
-        data = _in.read()
-        self.templates.append((source_path, data))
-
-        # Write back or the cache would not detect changes
-        out.write(data)
-
-    def output(self, _in, out, **kwargs):
-        base_path = self._find_base_path() + os.path.sep
+    def process_templates(self, out, hunks, **kwargs):
         namespace = self.namespace or 'window.JST'
 
         if self.bare is False:
-            out.write("(function(){\n    ")
+            out.write("(function(){\n")
 
         out.write("%s = %s || {};\n" % (namespace, namespace))
 
         if self.include_jst_script:
             out.write("%s\n" % _jst_script)
 
-        for path, contents in self.templates:
+        for name, hunk in self.iter_templates_with_base(hunks):
             # Make it a valid Javascript string. Is this smart enough?
-            contents = contents.replace('\n', '\\n').replace("'", r"\'")
+            contents = hunk.data().replace('\n', '\\n').replace("'", r"\'")
 
-            out.write("%s['%s'] = " % (namespace,
-                os.path.splitext(path[len(base_path):])[0]))
+            out.write("%s['%s'] = " % (namespace, name))
             if self.template_function is False:
                 out.write("'%s';\n" % (contents))
             else:
@@ -152,11 +161,6 @@ def output(self, _in, out, **kwargs):
         if self.bare is False:
             out.write("})();")
 
-    def _find_base_path(self):
-        """Hmmm. There should aways be some common base path."""
-        paths = [path for path, content in self.templates]
-        return JSTemplateFilter._find_base_path(self, paths)
-
 
 _jst_script = 'var template = function(str){var fn = new Function(\'obj\', \'var \
 __p=[],print=function(){__p.push.apply(__p,arguments);};\

src/webassets/merge.py

@@ -14,7 +14,7 @@
 
 
 __all__ = ('FileHunk', 'MemoryHunk', 'merge', 'FilterTool',
-           'MoreThanOneFilterError')
+           'MoreThanOneFilterError', 'NoFilters')
 
 
 # Log which is used to output low-level information about what the build does.
@@ -181,6 +181,10 @@ def __init__(self, message, filters):
         self.filters = filters
 
 
+class NoFilters(Exception):
+    pass
+
+
 class FilterTool(object):
     """Can apply filters to hunk objects, while using the cache.
 
@@ -283,7 +287,7 @@ def apply_func(self, filters, type, args, kwargs=None, cache_key=None):
         filters = [f for f in filters if getattr(f, type, None)]
         if not filters:  # Short-circuit
             log.debug('No filters have a "%s" method' % type)
-            return None
+            raise NoFilters()
 
         if len(filters) > 1:
             raise MoreThanOneFilterError(

tests/test_bundle_build.py

@@ -411,7 +411,7 @@ def test_concat(self):
         """
         class ConcatFilter(Filter):
             def concat(self, out, hunks, **kw):
-                out.write('%%%'.join([h.data() for h in hunks]))
+                out.write('%%%'.join([h.data() for h, info in hunks]))
         self.create_files({'a': '1', 'b': '2'})
         self.mkbundle('a', 'b', filters=ConcatFilter, output='out').build()
         assert self.get('out') == '1%%%2'

tests/test_bundle_various.py

@@ -555,7 +555,7 @@ def test_valid_url(self):
     def test_invalid_url(self):
         """If a bundle contains an invalid url, building will raise an error.
         """
-        assert_raises(BuildError,
+        assert_raises(IOError,
                       self.mkbundle('http://bar', output='out').build)
 
     def test_autorebuild_updaters(self):