Indenting a function to recursively find filenames

Question

I wrote this function that recursively traverses a given directory resolves the relative file-names to absolute file-names and yields the resolved file-name if it matches a given pattern. You can specify a callback function as well.

The first issue I have with this function is that it is a bit hard to comprehend, I think this problem can be reduced with proper indentation of the generator expressions.

The second thing I would like help with is, How do i reduce the repeated block of code?

import fnmatch
import os
import sys

def find(root, pattern, *callback):
    if not callback:
        for _, _, files in  ((root, _, (os.path.join(root, filename)
        for filename in files if fnmatch.fnmatch(filename, pattern)))
        for (root, _, files) in os.walk(root)):
            for filename in files:
                yield filename

    callback = callback[0]
    for _, _, files in  ((root, _, (os.path.join(root, filename)
    for filename in files if fnmatch.fnmatch(filename, pattern)))
    for (root, _, files) in os.walk(root)):
        for filename in files:
            yield callback(filename)



def cb(filename):
    print filename


for filename in find('/home/dwilson', '*.py', cb):
    pass

Answer 1

A good way would be to eliminate duplicate using a default callback that does nothing:

def walker(root, pattern):
    for _, _, files in  ((root, _, (os.path.join(root, filename)
        for filename in files if fnmatch.fnmatch(filename, pattern)))
        for (root, _, files) in os.walk(root)):
            for filename in files:
                yield filename

def find(root, pattern, callback=lambda v:v):
    for filename in walker(root, pattern):
        yield callback(filename)

Afterwards, we simplify the walker to

def walker(root, pattern):
    for (root, _, files) in os.walk(root):
        for filename in files:
            if fnmatch.fnmatch(filename, pattern):
                yield os.path.join(root, filename)

and have a much more readable thing than before.

Answer 2

I think I need my own space to explain "my approach" with default lambda - thanks to glglgl for hospitality :) .

It saves you from having to check condition in the function. And the overhead? Not that much (here comes dis, which I - alas! - nearly never use)

dis.dis(lambda v: v)
  1           0 LOAD_FAST                0 (v)
              3 RETURN_VALUE  

But how bad is it - compared to other approaches? See below

>>> def test1(param, callback=lambda v:v):
    return callback(param)

>>> def test2(param, *callback):
    if callback:
        return callback[0](param)
    else:
        return param

>>> def test3(param, callback=None):
    if callback is not None:
        return callback(param)
    else:
        return param

>>> dis.dis(test1)
  2           0 LOAD_FAST                1 (callback)
              3 LOAD_FAST                0 (param)
              6 CALL_FUNCTION            1
              9 RETURN_VALUE        
>>> dis.dis(test2)
  2           0 LOAD_FAST                1 (callback)
              3 POP_JUMP_IF_FALSE       20

  3           6 LOAD_FAST                1 (callback)
              9 LOAD_CONST               1 (0)
             12 BINARY_SUBSCR       
             13 LOAD_FAST                0 (param)
             16 CALL_FUNCTION            1
             19 RETURN_VALUE        

  5     >>   20 LOAD_FAST                0 (param)
             23 RETURN_VALUE        
             24 LOAD_CONST               0 (None)
             27 RETURN_VALUE        
>>> dis.dis(test3)
  2           0 LOAD_FAST                1 (callback)
              3 LOAD_CONST               0 (None)
              6 COMPARE_OP               9 (is not)
              9 POP_JUMP_IF_FALSE       22

  3          12 LOAD_FAST                1 (callback)
             15 LOAD_FAST                0 (param)
             18 CALL_FUNCTION            1
             21 RETURN_VALUE        

  5     >>   22 LOAD_FAST                0 (param)
             25 RETURN_VALUE        
             26 LOAD_CONST               0 (None)
             29 RETURN_VALUE        

Surprise, surprise, None loses by 2 bytecodes (did not see it coming), still - it is cleaner that *callback, which in semantically misleading, since it implies possibility of more than one value. And default callback overhead is less than 25% of any

As for run-time impact - try timing runs with different implementations, and see for yourself

Answer 3

Well.. I got rid of the duplicated block of code but the list comprehensions are still hard to follow, And i wrote them...

I don't like how for each iteration of the loop the function has to check to see if a callback was given.

import fnmatch
import os
import sys

def find(root, pattern, *callback):

    for _, _, files in  ((root, _, (os.path.join(root, filename)
    for filename in files if fnmatch.fnmatch(filename, pattern)))
    for (root, _, files) in os.walk(root)):
        for filename in files:
            if not callback:
                yield filename
            else:
                yield callback[0](filename)

def print_filename(filename):
    print filename


for filename in find('/', '*.c', print_filename):
    pass

Answer 4

I took the liberty of using shortened names due to the constraints of interactivity but I hope that you can decide if this suggestion is good enough for your use case.

Here follows a (slightly edited) ipython interactive session.

In [38]: from os.path import join as j
In [39]: from os import walk as w
In [40]: from fnmatch import fnmatch as fm
In [41]: def x(r,p,*c):
    g = (j(r,fn) for r, dl, fl in w(r) for fn in fl if fm(fn,p))
    return (c[0](fn) for fn in g) if c else g
   ....: 
In [42]: x('bin','*py*')
Out[42]: <generator object <genexpr> at 0x7fdf7411cc30>
In [43]: list(x('bin','*py*'))
Out[43]: 
['bin/pythontex_engines.py',
 ...,
 'bin/pythontex2.py']
In [44]: x('bin','*py*',lambda s:s.split('/'))
Out[44]: <generator object <genexpr> at 0x7fdf7411cd20>
In [45]: list(x('bin','*py*',lambda s:s.split('/')))
Out[45]: 
[['bin', 'pythontex_engines.py'],
 ...,
 ['bin', 'pythontex2.py']]
In [46]:

As you have noticed, my code doesn't yield a file name but rather it returns a generator object... that's not exactly what you want but its use in a for loop is the same and other use cases can be easily taken into account.

Another take at it

As the question was about proper indentation, I'm ambigously, ashamedly proud of this new attempt of mine...

def find(root, pattern, *callback):

    from os.path import join
    from os import walk
    from fnmatch import fnmatch

    gen = (join(cr, fn)

#   Trompe-l'oeil code

    for cr, dl, fl in walk(root)
        for fn in fl
            if fnmatch(fn, pattern))
#               join(cr, fn) )

    return (callback[0](fn) for fn in gen) if callback else gen