Filtering logic

Question

I have a FileAsset class. I have a set of 'filters' with which I determine if a FileAsset should be written to my database. Below is a telescoped version of how the filters work now. My big question is this: Is there a better way? Any advice would be awesome!

(By the way, the Attribute key in the filters allows me to test other attributes of the FileAsset class which I have not demonstrated for simplicity's sake, but the concept is the same.)

import os
from fnmatch import fnmatch
import operator

class FileAsset:
    def __init__(self, filename):
        self.filename = filename

    @property
    def is_asset(self):
        filters = [
                {'Test':'matches','Attribute':'filename','Value':'*'},
                {'Test':'ends with','Attribute':'filename','Value':'txt'},
                {'Test':'does not end with','Attribute':'filename','Value':'jpg'}]
        results = []
        try:
            results.append(file_filter(self, filters))
        except Exception as e:
            results.append(True)
        return True in results

    def __repr__(self):
        return '<FileAsset: %s>' % self.filename

def file_filter(file_asset,filters):
    results = []
    for f in filters:
        try:
            attribute = operator.attrgetter(f['Attribute'])(file_asset)
            try:
                result = filter_map(f['Test'])(attribute,f['Value'])
                results.append(result)
            except Exception as e:
                print e
                results.append(False)
        except AttributeError as e:
            print e
            results.append(False)
    return not False in results


def filter_map(test):
    if test == u'is file':
        return lambda x, y: os.path.isfile(x)
    elif test == u'contains':
        return operator.contains
    elif test == u'matches':
        return lambda x, y: fnmatch(x,y)
    elif test == u'does not contain':
        return lambda x, y: not y in x
    elif test == u'starts with':
        return lambda x, y: x.startswith(y)
    elif test == u'does not start with':
        return lambda x, y: not x.startswith(y)
    elif test == u'ends with':
        return lambda x, y: x.endswith(y)
    elif test == u'does not end with':
        return lambda x, y: not x.endswith(y)
    # etc etc


fa1 = FileAsset('test.txt')
fa2 = FileAsset('test.jpg')

print '%s goes to db: %s' % (fa1.filename, fa1.is_asset)
print '%s goes to db: %s' % (fa2.filename, fa2.is_asset)

Answer 1

You have written a small DSL to map tests to Python operations. The DSL uses dicts which imposes some limitations. Try this approach instead:

filter1 = FileFilter().filenameEndsWith('.txt').build()

names = ['test.txt', 'test.jpg']
to_save = filter(filter1, names)

How does that work? FileFilter is a builder. Builders are like flexible factories. Internally, the code could look like so:

class FileFilter:
    def __init__(self):
        self.conditions = []

    def filenameEndsWith(self, pattern):
        def c(filename):
            return filename.endswith(pattern)
        self.conditions.append(c)
        return self

    def build(self):
        def f(filename, conditions=self.conditions):
            for c in conditions:
                if not c(filename): return False
            return True
        return f

Notes: This is just an example. You will probably use lambdas and return an object that implements the filter API in build()

Answer 2

import os
from fnmatch import fnmatch
import operator

class FileAsset:
    def __init__(self, filename):
        self.filename = filename

    @property
    def is_asset(self):
        filters = [
                {'Test':'matches','Attribute':'filename','Value':'*'},
                {'Test':'ends with','Attribute':'filename','Value':'txt'},
                {'Test':'does not end with','Attribute':'filename','Value':'jpg'}]
        results = []
        try:
            results.append(file_filter(self, filters))
        except Exception as e:

Don't ever do this. Python will throw exceptions in the case of many bugs in your code. If you catch all exceptions, you'll eventually catch and hide a legitimate bug. Can this even happen? Don't you catch all the interesting exceptions in file_filter?

            results.append(True)

Why do you make a list if you are only going to store a single value in it? It seems to you should just be doing return file_filter(...

        return True in results

Why do you construct a list, only to put a single item in it.

    def __repr__(self):
        return '<FileAsset: %s>' % self.filename

def file_filter(file_asset,filters):
    results = []
    for f in filters:

I recommend spelling out filter, just for the extra clarity.

        try:
            attribute = operator.attrgetter(f['Attribute'])(file_asset)

Just use attribute = getattr(file_asset, f['Attribute']), there is no point in introducing operator here.

            try:
                result = filter_map(f['Test'])(attribute,f['Value'])

This really looks like filter_map should take three arguments, and not be split across two function calls

                results.append(result)
            except Exception as e:

If something goes wrong, do you really want to carry on as though nothing has happened? If the requested filter was invalid in some way an exception should be raised

                print e
                results.append(False)

And why in the yellow tarnekey chicken is this False, when last time an exception made it True?

        except AttributeError as e:
            print e
            results.append(False)

This is more acceptable, because you are attempting to specifically catch one exception, that of the attrgetter above. But you should the rest of the logic into an else block on the exception. But again, you should really throw an exception to indicate the filter was invalid.

    return not False in results

Instead use, return all(results)

def filter_map(test):
    if test == u'is file':
        return lambda x, y: os.path.isfile(x)

This would be a great place to use a dictionary. Have the dictionary from the test strings to the lambdas, and then you look them up.

Here is my really big problem with your code. You've build this filtering logic system and created something harder to use, less powerful, and slower then just using python.

Consider:

@property
def is_asset(self):
    return (
         fnmatch(self.filename, '*'),
         and self.filename.endswith('txt'),
         and not self.filename.endswith('jpg')
    )

It seems to me that this is: shorter, easier to follow, and more flexible then your system. And that's not even considering a better way to implement this

@property
def extension(self):
    return os.path.splitext(self.filename)[-1]

@property
def is_asset(self):
    return self.extension == 'txt'

So I can't see how your system makes any sort of sense. But perhaps it makes more sense in the broader context of your system. If so, I'd like to see more of the broader context because I have trouble believing it.