Code review

querybuilder/constants.py

@@ -84,6 +84,9 @@ def handles(self, f):
         f.operator = self
         return f
 
+
+# Does it make sense to just declare these in the class? At the very least, helps with PyCharm's
+# jump to definition feature
 Operators.unary_comparisons = frozenset({
     Operators.IS_NULL,
     Operators.IS_NOT_NULL

querybuilder/filters.py

@@ -271,10 +271,12 @@ def equal(self, lop, rop):
     def not_equal(self, lop, rop):
         return not self.equal(lop, rop)
 
+    # Does this belong on the base class? This doesn't fit with scalars
     @Operators.IN.handles
     def _in(self, lop, rop):
         return lop in rop
 
+    # Does this belong on the base class? This doesn't fit with scalars
     @Operators.NOT_IN.handles
     def not_in(self, lop, rop):
         return not self._in(lop, rop)
@@ -306,6 +308,7 @@ def between(self, op, minop, maxop):
     def not_between(self, op, minop, maxop):
         return not self.between(op, minop, maxop)
 
+    # Does this belong on the base class? This doesn't fit with scalars
     @Operators.CONTAINS.handles
     def contains(self, lop, rop):
         return self._in(lop, rop)
@@ -320,6 +323,7 @@ def is_not_null(self, op):
 
 
 class TypedFilter(Filter):
+    # Would it make sense to use abstract properties?
     TYPE = NotImplemented
     OPERATORS = NotImplemented
     OPTIONS = NotImplemented
@@ -328,6 +332,7 @@ def __init__(self, *args, **kwargs):
         kwargs.update(type=self.TYPE)
         assert self.TYPE is not NotImplemented, 'TYPE must be declared in the subclass'
 
+        # BUG: If self.OPERATORS
         if self.OPTIONS is not NotImplemented:
             kwargs.setdefault('operators', tuple(self.OPERATORS))
 
@@ -423,7 +428,6 @@ class DateFilter(TypedFilter):
 
     # TODO add default validator
 
-
 class TimeFilter(TypedFilter):
     TYPE = Types.TIME
 

querybuilder/rules.py

@@ -70,11 +70,17 @@ def validate(self, value):
             pass
 
 
+# Since you were wanting a better entry point, would it make more sense to have a `RulesEngine`
+# class or something similar? You'd pass the json to that and it'd handle making the rules as
+# necessary. Then this class doesn't need to worry about serder at all.
+# Not that much of an improvement, but at least it's a more obvious entry point.
 class Rule(object):
     def __init__(self, rule):
         '''
         Args:
+            What's the rule object? A dict?
             rule: The rule object
+            # Looks like these params have been removed
             operators: Enum of operators, this allows for adding custom operators
             inputs: Enum of inputs, this allows for adding custom inputs
             types: Enum of types, this allows for adding custom types
@@ -83,8 +89,9 @@ def __init__(self, rule):
         self.is_group = False
         self.is_empty = False  # note that an empty rule evaluates as true
 
+        # Could you add some more documentation on what these 3 types of rules are?
         if rule.get('empty'):
-            # some rules
+            # some rules what?
             self.is_empty = True
         elif 'condition' and 'rules' in rule:
             self.is_group = True