6
\$\begingroup\$

I have a requirement to be able to filter a list of strings by both inclusion and exclusion patterns (using fnmatch-style wildcards), of which there can be many.

For example, given the set of values:

['a1', 'a2', 'a3', 'b1', 'b2', 'b3', 'c1', 'c2', 'c3']

And these inclusion filters:

['a*', 'b*']

It should return:

['a1', 'a2', 'a3', 'b1', 'b2', 'b3']

It should also support using exclusion filters, e.g. ['c*'] to get the same result.

Lastly, using both inclusion and exclusion filters, the exclusion filters should take precedence in case of a conflict.

Here is my code and tests:

  • extrafilters.py

    import fnmatch
    
    def superFilter(names, inclusion_patterns=[], exclusion_patterns=[]):
        """Enhanced version of fnmatch.filter() that accepts multiple inclusion and exclusion patterns.
        If only inclusion_patterns is specified, only the names which match one or more patterns are returned.
        If only exclusion_patterns is specified, only the names which do not match any pattern are returned.
        If both are specified, the exclusion patterns take precedence.
        If neither is specified, the input is returned as-is."""
        included = multiFilter(names, inclusion_patterns) if inclusion_patterns else names
        excluded = multiFilter(names, exclusion_patterns) if exclusion_patterns else []
        return set(included) - set(excluded)
    
    def multiFilter(names, patterns):
        """Generator function which yields the names that match one or more of the patterns."""
        for name in names:
            for pattern in patterns:
                if fnmatch.fnmatch(name, pattern):
                    yield name
    
  • extrafilters_test.py

    import unittest
    from extrafilters import superFilter, multiFilter
    
    class multiFilterTests(unittest.TestCase):
        def setUp(self):
            self.names = ['a1', 'a2', 'a3', 'b1', 'b2', 'b3', 'c1', 'c2', 'c3']
    
        def test_patterns(self):
            patterns = ['a*', 'b*']
            expected = ['a1', 'a2', 'a3', 'b1', 'b2', 'b3']
            self.assertItemsEqual(list(multiFilter(self.names, patterns)), expected,
                'should include only the names which match one or more of the patterns')
    
    class superFilterTests(unittest.TestCase):
        def setUp(self):
            self.names = ['a1', 'a2', 'a3', 'b1', 'b2', 'b3', 'c1', 'c2', 'c3']
    
        def test_no_patterns(self):
            expected = self.names
            self.assertItemsEqual(superFilter(self.names), expected,
                'should be the same as the input')
    
        def test_inclusion(self):
            include = ['a*', 'b*']
            expected = ['a1', 'a2', 'a3', 'b1', 'b2', 'b3']
            self.assertItemsEqual(superFilter(self.names, include), expected,
                'should include only the inclusions')
    
        def test_exclusion(self):
            exclude = ['c*']
            expected = ['a1', 'a2', 'a3', 'b1', 'b2', 'b3']
            self.assertItemsEqual(superFilter(self.names, exclusion_patterns=exclude), expected,
                'should include all but the exclusions')
    
        def test_inclusions_and_exclusions(self):
            include = ['a*', 'b*']
            exclude = ['b*']
            expected = ['a1', 'a2', 'a3']
            self.assertItemsEqual(superFilter(self.names, include, exclude), expected,
                'should include the inclusions minus the exclusions')
    
    if __name__ == "__main__":
        unittest.main()
    

What corner cases will break this, and how is the clarity of the code (function names, docstrings, etc.)? Are the unit tests complete and detailed enough or perhaps too much? Any other style or quality tips are also appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! You passed through the First Post Queue with flying colors! \$\endgroup\$ – RubberDuck Dec 24 '14 at 1:03
3
\$\begingroup\$

You can use fnmatch.filter to simplify multiFilter:

for pattern in patterns:
    for name in fnmatch.filter(names, pattern):
        yield name

This still allows an element to be yielded several times, so instead you might want to do:

for name in names:
    if any(fnmatch.fnmatch(name, pattern) for pattern in patterns):
        yield name

You can also avoid creating a second set by using .difference:

return set(included).difference(excluded)

I would probably stick with the original, though. I only mention it because a fair number of people don't know about it and sometimes it does matter.

Your naming breaks PEP8. Don't let unittest fool you; unittest was written by heathens before the style guide was standardized.

A quick touch-up is

import fnmatch

def super_filter(names, inclusion_patterns=[], exclusion_patterns=[]):
    """
    Enhanced version of fnmatch.filter() that accepts multiple inclusion and exclusion patterns.

    If only inclusion_patterns is specified, only the names which match one or more patterns are returned.
    If only exclusion_patterns is specified, only the names which do not match any pattern are returned.
    If both are specified, the exclusion patterns take precedence.
    If neither is specified, the input is returned as-is.
    """
    included = multi_filter(names, inclusion_patterns) if inclusion_patterns else names
    excluded = multi_filter(names, exclusion_patterns) if exclusion_patterns else []
    return set(included) - set(excluded)

def multi_filter(names, patterns):
    """Generator function which yields the names that match one or more of the patterns."""
    for name in names:
        if any(fnmatch.fnmatch(name, pattern) for pattern in patterns):
            yield name

I would also consider changing the description; it's obvious from introspection that inclusion_patterns and exclusion_patterns default to empty, which is counter-intuitive. I would change it to

def super_filter(names, inclusion_patterns=('*',), exclusion_patterns=()):
    """
    Enhanced version of fnmatch.filter() that accepts multiple inclusion and exclusion patterns.

    Filter the input names by choosing only those that are matched by
    some pattern in inclusion_patterns _and_ not by any in exclusion_patterns.
    """
    included = multi_filter(names, inclusion_patterns)
    excluded = multi_filter(names, exclusion_patterns)
    return set(included) - set(excluded)

Your tests cover the general case fine but they don't check edge-cases; what happens with 0-length inputs? What about complicated patterns? Do you ever check precedence, despite having mentioned it?

\$\endgroup\$
  • \$\begingroup\$ Thanks for your helpful answer. Being new to Python, I didn't understand why the comma is necessary in inclusion_patterns=('*',) in your last block of code. Discovered the for statement in multi_filter won't work with a tuple of one element. Might be nice to add an if statement to add the comma if it isn't present, like this answer instructs. \$\endgroup\$ – dnlbrky Sep 3 '15 at 20:25
  • \$\begingroup\$ I agree with bobince here. The comma makes a tuple, and it's a bad idea the majority of times to wrap single values automatically. If a function returns a stand-alone value in special cases, the correct response is to fix or wrap that function, not make all your other functions do extra work. \$\endgroup\$ – Veedrac Sep 3 '15 at 20:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.