Refactored unit tests and improved dotty output.

BUG=
R=joachim.metz@gmail.com

Review URL: https://codereview.appspot.com/250890043.

efilter/README.md

@@ -18,7 +18,7 @@ Let's have a class in our custom project:
 
 	class Customer(object):
 		"""My awesome Customer business logic class."""
-
+		
 		@property
 		def name(self):
 			#...
@@ -37,7 +37,7 @@ We'd like to filter this class's instances using EFILTER, but we need a way to
 EFILTER uses the IAssociative protocol to access members of the objects its
 asked to filter. Implementing a protocol lets EFILTER know how each type should
 be accessed:
-	
+
 	from efilter.protocols import associative
 	associative.IAssociative.implement(
 		for_type=Customer,  # This is how you access Customer's data.
@@ -56,7 +56,7 @@ be accessed:
 			# accessed.
 			associative.getkeys: lambda _: ("name", "age", "admin")
 		})
-	
+
 Now we can filter the Customer class:
 
 	query = Query("name == 'Bob'")

efilter/engine.py

@@ -41,12 +41,19 @@ class NopAppDelegate(object):
 
 
 class Engine(object):
-    """Base class representing the various behaviors of the EFILTER AST."""
+    """Base class representing the various behaviors of the EFILTER AST.
+
+    Attributes:
+        query: The query object this is running on.
+        application_delegate: The application delegate - should be the same as
+            the one on the query.
+    """
     __metaclass__ = abc.ABCMeta
 
     ENGINES = {}
 
     def __init__(self, query, application_delegate=None):
+        super(Engine, self).__init__()
         self.query = query
         if application_delegate:
             self.application_delegate = application_delegate
@@ -55,7 +62,7 @@ def __init__(self, query, application_delegate=None):
 
     @abc.abstractmethod
     def run(self, *_, **__):
-        pass
+        """Execute this engine and return its result."""
 
     @classmethod
     def register_engine(cls, subcls, shorthand=None):
@@ -73,7 +80,30 @@ def get_engine(cls, shorthand):
 
 
 class VisitorEngine(Engine):
-    """Engine that implements the visitor pattern."""
+    """Engine that implements the visitor pattern.
+
+    Visitor engines start by calling self.visit(self.root). The default
+    implementation of self.visit will walk the MRO of the expression it got
+    and find the best available handler in the form of visit_<classname>.
+
+    (This ends up trying visit_Expression as last resort and then throwing a
+    TypeError if no handlers are available.)
+
+    The actual handlers themselves are implemented by subclasses providing
+    different behaviors (e.g. matcher, analyzer, etc.). Each handler is
+    responsible for evaluating the branch it's passed, usually by recursively
+    calling self.visit on any children and then combining the results of those
+    sub-branches according to its own logic.
+
+    For example, to implement expression.Sum (add up numbers), one would do:
+
+        def visit_Sum(self, expr):
+            result = 0
+            for branch in expr.children:
+                result += self.visit(branch)
+
+            return result
+    """
 
     def __hash__(self):
         return hash((type(self), self.query))
@@ -85,11 +115,26 @@ def __ne__(self, other):
         return not self.__eq__(other)
 
     def run(self, *_, **kwargs):
+        """Visitors by default visit the root of the query."""
         super(VisitorEngine, self).run()
         self.node = self.query.root
         return self.visit(self.node, **kwargs)
 
     def fall_through(self, node, engine_shorthand, **kwargs):
+        """A visitor only implementing part of the AST can delegate with this.
+
+        If a visitor engine only implemenets a subset of the AST language, it
+        becomes useful to delegate to other visitor classes for subbranches the
+        original visitor cannot handle.
+
+        Arguments:
+            node: The subbranch to be handled by the other visitor.
+            engine_shorthand: The shorthand name of the other visitor.
+            kwargs: Are passed on verbatim.
+
+        Returns:
+            Whatever the other visitor engine returns.
+        """
         engine_cls = type(self).get_engine(engine_shorthand)
         if not issubclass(engine_cls, VisitorEngine):
             raise TypeError(
@@ -100,6 +145,7 @@ def fall_through(self, node, engine_shorthand, **kwargs):
         return engine.visit(node, **kwargs)
 
     def visit(self, node, **kwargs):
+        """Visit the AST node by calling as specific a handler as we have."""
         # Walk the MRO and try to find a closest match for handler.
         for cls in type(node).mro():
             handler_name = "visit_%s" % cls.__name__

efilter/engines/analyzer.py

@@ -36,6 +36,8 @@
 class RuleAnalyzer(engine.VisitorEngine):
     """This is a rule-driven analyzer that gets a list of symbols and indexing.
 
+    This class follows the visitor pattern. See documentation on VisitorEngine.
+
     The analyzer will produce a list of symbols required by the query (based on
     the Bindings/variables) and recommend a list of Bindings suitable for
     building an equivalence-based index (based on Equivalence expressions in
@@ -150,4 +152,5 @@ def visit_Equivalence(self, expr, **kwargs):
 
         return Analysis(symbols, indexables)
 
+
 engine.Engine.register_engine(RuleAnalyzer, "analyzer")

efilter/engines/dotty_output.py

@@ -40,7 +40,10 @@ def build_operator_lookup(*tables):
 
 
 class DottyOutput(engine.VisitorEngine):
-    """Produces equivalent Dotty output to the AST."""
+    """Produces equivalent Dotty output to the AST.
+
+    This class follows the visitor pattern. See documentation on VisitorEngine.
+    """
 
     TOKENS = build_operator_lookup(dotty.INFIX, dotty.PREFIX)
 
@@ -51,7 +54,8 @@ def visit_Let(self, expr):
         right = self.visit(rhs)
         token = "."
 
-        if not isinstance(expr.lhs, expression.ValueExpression):
+        if not isinstance(expr.lhs, (expression.ValueExpression,
+                                     expression.Let)):
             left = "(%s)" % left
             token = " where "
 
@@ -62,6 +66,12 @@ def visit_Let(self, expr):
 
         return token.join((left, right))
 
+    def visit_LetAny(self, expr):
+        return "any %s" % self.visit_Let(expr)
+
+    def visit_letEach(self, expr):
+        return "each %s" % self.visit_Let(expr)
+
     def visit_Literal(self, expr):
         return repr(expr.value)
 
@@ -79,4 +89,5 @@ def visit_VariadicExpression(self, expr):
         separator = " %s " % token
         return separator.join(self.visit(x) for x in expr.children)
 
+
 engine.Engine.register_engine(DottyOutput, "dotty_output")

efilter/engines/hinter.py

@@ -28,6 +28,11 @@
 class Hinter(engine.VisitorEngine):
     """Retargets the query to apply to a subexpression, to be used for hinting.
 
+    This class follows the visitor pattern. See documentation on VisitorEngine.
+
+    Discussion of mechanism and rationale:
+    ======================================
+
     Bear with me on this one.
 
     As with any database-like system, certain EFILTER queries can be satisfied

efilter/engines/matcher.py

@@ -31,7 +31,10 @@
 
 
 class ObjectMatcher(engine.VisitorEngine):
-    """Given a query and bindings will evaluate the query."""
+    """Given a query and bindings will evaluate the query.
+
+    This class follows the visitor pattern. See documentation on VisitorEngine.
+    """
 
     # run() sets this to the sort value for the latest object matched.
     latest_sort_order = None
@@ -223,4 +226,5 @@ def visit_PartialOrderedSet(self, expr, **_):
 
         return True
 
+
 engine.Engine.register_engine(ObjectMatcher, "filter")

efilter/engines/normalizer.py

@@ -27,6 +27,8 @@
 class Normalizer(engine.VisitorEngine):
     """Optimizes the AST for better performance and simpler structure.
 
+    This class follows the visitor pattern. See documentation on VisitorEngine.
+
     The returned query will be logically equivalent to what was provided but
     transformations will be made to flatten and optimize the structure. This
     engine works by recognizing certain patterns and replacing them with nicer
@@ -60,6 +62,15 @@ def run(self, *args, **kwargs):
         return self.query.subquery(expr)
 
     def visit_Let(self, expr, **kwargs):
+        """Rotate repeated let-forms so they cascade on the RHS.
+
+        Basic let-forms should be rotated as follows:
+        (let (let x y) (...)) => (let x (let y) (...))
+
+        These are functionally equivalent, but the latter is easier to follow.
+
+        Returns rotated Let instance.
+        """
         lhs = self.visit(expr.lhs, **kwargs)
         rhs = self.visit(expr.rhs)
 
@@ -71,13 +82,38 @@ def visit_Let(self, expr, **kwargs):
 
         return type(expr)(lhs, rhs)
 
+    def visit_LetAny(self, expr, **kwargs):
+        """let-any|let-each forms are not cascaded.
+
+        This is basically a pass-through function.
+        """
+        lhs = self.visit(expr.lhs, **kwargs)
+        rhs = self.visit(expr.rhs, **kwargs)
+        return type(expr)(lhs, rhs)
+
+    def visit_LetEach(self, expr, **kwargs):
+        return self.visit_LetAny(expr, **kwargs)
+
     def visit_Expression(self, expr, **_):
         return expr
 
     def visit_BinaryExpression(self, expr, **kwargs):
         return self.visit_VariadicExpression(expr, **kwargs)
 
     def visit_VariadicExpression(self, expr, **kwargs):
+        """Pass through n-ary expressions, and eliminate empty branches.
+
+        Variadic and binary expressions recursively visit all their children.
+
+        If all children are eliminated then the parent expression is also
+        eliminated:
+
+        (& [removed] [removed]) => [removed]
+
+        If only one child is left, it is promoted to replace the parent node:
+
+        (& True) => True
+        """
         children = []
         for child in expr.children:
             branch = self.visit(child, **kwargs)
@@ -97,4 +133,5 @@ def visit_VariadicExpression(self, expr, **kwargs):
 
         return type(expr)(*children)
 
+
 engine.Engine.register_engine(Normalizer, "normalizer")

efilter/engines/test_dotty_output.py

@@ -21,3 +21,8 @@ def testWhere(self):
         self.assertOutput(
             original="Process.parent where (name == 'foo' and pid == 5)",
             output="Process.parent where (name == 'foo' and pid == 5)")
+
+    def testAnywhere(self):
+        self.assertOutput(
+            original="any Process.parent where (name == 'foo')",
+            output="any Process.parent where (name == 'foo')")

efilter/engines/test_hinter.py

@@ -1,54 +1,58 @@
-import unittest
+from efilter import testlib
 
-from efilter import query
-
-
-class HinterTest(unittest.TestCase):
-    def parseQuery(self, source, syntax="dotty"):
-        return query.Query(source, syntax=syntax)
-
-    def assertHinted(self, source, selector, expected, syntax="dotty"):
-        hinted = self.parseQuery(source).run_engine("hinter",
-                                                    selector=selector,
-                                                    syntax=syntax)
-        baseline = self.parseQuery(expected)
-        self.assertEqual(hinted, baseline)
 
+class HinterTest(testlib.EngineTestCase):
     def testNop(self):
-        self.assertHinted("Process.name == 'init'", None,
-                          "Process.name == 'init'")
+        self.assertTransform(
+            engine="hinter",
+            original="Process.name == 'init'",
+            expected="Process.name == 'init'",
+            selector=None)
 
     def testBasic(self):
-        self.assertHinted("Process.name == 'init'", "Process",
-                          "name == 'init'")
+        self.assertTransform(
+            engine="hinter",
+            original="Process.name == 'init'",
+            expected="name == 'init'",
+            selector="Process")
 
     def testMulti(self):
-        self.assertHinted("Process.parent.Process.name == 'init'",
-                          "Process.parent",
-                          "Process.name == 'init'")
+        self.assertTransform(
+            engine="hinter",
+            original="Process.parent.Process.name == 'init'",
+            selector="Process.parent",
+            expected="Process.name == 'init'")
 
     def testNested(self):
-        self.assertHinted("Process.parent.Process.name == 'init' "
-                          "and Process.pid > 10",
-                          "Process.parent",
-                          "Process.name == 'init'")
+        self.assertTransform(
+            engine="hinter",
+            original=("Process.parent.Process.name == 'init' "
+                      "and Process.pid > 10"),
+            selector="Process.parent",
+            expected="Process.name == 'init'")
 
     def testAndNested(self):
-        self.assertHinted("Process.parent.Process.name == 'init' "
-                          "and Process.parent.Process.pid > 10",
-                          "Process.parent",
-                          "Process.name == 'init' and Process.pid > 10")
+        self.assertTransform(
+            engine="hinter",
+            original=("Process.parent.Process.name == 'init' "
+                      "and Process.parent.Process.pid > 10"),
+            selector="Process.parent",
+            expected="Process.name == 'init' and Process.pid > 10")
 
     def testNestedWithComplement(self):
-        self.assertHinted("Process.parent.Process.name != 'init' "
-                          "and not Process.parent.Process.pid > 10",
-                          "Process.parent",
-                          "Process.name != 'init' and not Process.pid > 10")
+        self.assertTransform(
+            engine="hinter",
+            original=("Process.parent.Process.name != 'init' "
+                      "and not Process.parent.Process.pid > 10"),
+            selector="Process.parent",
+            expected="Process.name != 'init' and not Process.pid > 10")
 
     def testLegacy(self):
-        self.assertHinted(
-            "MemoryDescriptor.process where (Process.command == 'Adium')"
-            " and 'execute' in MemoryDescriptor.permissions"
-            " and 'write' in MemoryDescriptor.permissions",
-            "MemoryDescriptor.process",
-            "Process.command == 'Adium'")
+        self.assertTransform(
+            engine="hinter",
+            original=("MemoryDescriptor.process where "
+                      "(Process.command == 'Adium') "
+                      "and 'execute' in MemoryDescriptor.permissions "
+                      "and 'write' in MemoryDescriptor.permissions"),
+            selector="MemoryDescriptor.process",
+            expected="Process.command == 'Adium'")