breaking from loop

[Ritesh Raj Sarraf]

Hi,

Following is the code:

def walk_tree_copy(sRepository, sFile, sSourceDir, bFound = None):
try:
if sRepository is not None:
for name in os.listdir(sRepository):
path = os.path.join(sRepository, name)
if os.path.isdir(path):
walk_tree_copy(path, sFile, sSourceDir, bFound)
elif name.endswith('.foo') or name.endswith('.bar'):
if name == sFile:
try:
shutil.copy(path, sSourceDir)
except IOError, (errno, errstring):
errfunc(errno, errstring)
except shutil.Error:
print name + " is available in " +
sSourceDir + "Skipping Copy!"
bFound = True
break
return bFound
except OSError, (errno, strerror):
print errno, strerror

This function allows me to walk into a directory based tree and search
for files ending with .foo and .bar. My requirement is to copy
..foo/.bar.
Say in directory temp=>test=>test.bar is found. So shutil.copy will
copy it. I want that once the copy is done, it should make bFound =
True and get out.
But since test directory is under temp, work_tree_copy makes two calls
of the same function _but_ break only is able to get out from the inner
call.

Where am I wrong in this code ? Is there a better way to implement it ?

Regards,
rrs

 

[Sybren Stuvel]

Ritesh Raj Sarraf enlightened us with:

bFound = True
break
return bFound

I see two weird things. First of all, the retun statement won't be
reached due to the break before it. Let's assume the break isn't
needed, and you want just the return. Why set the variable if you're
going to return anyway? I'd change those three lines to "return True".

But since test directory is under temp, work_tree_copy makes two
calls of the same function _but_ break only is able to get out from
the inner call.

Raise an exception and catch it at the outer level.

Sybren

 

[Peter Otten]

Following is the code:

def walk_tree_copy(sRepository, sFile, sSourceDir, bFound = None):
try:
if sRepository is not None:
for name in os.listdir(sRepository):
path = os.path.join(sRepository, name)
if os.path.isdir(path):
walk_tree_copy(path, sFile, sSourceDir, bFound)
elif name.endswith('.foo') or name.endswith('.bar'):
if name == sFile:

Pick /one/ the conditions

- file name must be ...
- file must end with ... or ...

try:
shutil.copy(path, sSourceDir)
except IOError, (errno, errstring):
errfunc(errno, errstring)
except shutil.Error:
print name + " is available in " +
sSourceDir + "Skipping Copy!"
bFound = True
break
return bFound
except OSError, (errno, strerror):
print errno, strerror

This function allows me to walk into a directory based tree and search
for files ending with .foo and .bar. My requirement is to copy
.foo/.bar.
Say in directory temp=>test=>test.bar is found. So shutil.copy will
copy it. I want that once the copy is done, it should make bFound =
True and get out.
But since test directory is under temp, work_tree_copy makes two calls
of the same function _but_ break only is able to get out from the inner
call.

Where am I wrong in this code ? Is there a better way to implement it ?

An implementation on top of os.walk() can simplify things a bit:

import os
import shutil

def files(root):
for path, folders, files in os.walk(root):
for file in files:
yield path, file

def copy_first_match(repository, filename, dest_dir): # aka walk_tree_copy()
for path, file in files(repository):
if file == filename:
shutil.copy(os.path.join(path, file), dest_dir)
return True
return False

files() and os.walk() take care of the recursion, and you can just break or
return from the loop in copy_first_match().

Peter

 

[bruno at modulix]

Hi,

Following is the code:

def walk_tree_copy(sRepository, sFile, sSourceDir, bFound = None):
try:
if sRepository is not None:

You're being overly defensive here. Passing None as first arg is clearly
a programming error, so the sooner you detect it the better. Hence, just
don't test, and let exceptions propagate.

for name in os.listdir(sRepository):
path = os.path.join(sRepository, name)
if os.path.isdir(path):
walk_tree_copy(path, sFile, sSourceDir, bFound)
elif name.endswith('.foo') or name.endswith('.bar'):
if name == sFile:

Why do you *first* test on the extension, and *then* on the whole name ?
The boolean expression:
(name.endswith('.foo') or name.endswith('.bar')) and name == sFile
logically implies that :
sFile.endswith('.foo') or sFile.endswith('.bar')

So the first test is pretty useless...

Also, why hardcode such tests ? Python makes it easy to write generic
(hence reusable) code.

try:
shutil.copy(path, sSourceDir)
except IOError, (errno, errstring):
errfunc(errno, errstring)

??? What is this function doing ?

except shutil.Error:
print name + " is available in " +
sSourceDir + "Skipping Copy!"

Don't assume the cause of the exception. FWIW, try the following snippet:
.... shutil.copy('test.txt', 'testcopy.txt')
....
Also, stdout is meant to be used for normal program outputs. Other
messages should go to stderr.

bFound = True
break
return bFound

Err... this last statement won't be executed.

except OSError, (errno, strerror):
print errno, strerror

This function allows me to walk into a directory based tree and search
for files ending with .foo and .bar. My requirement is to copy
.foo/.bar.
Say in directory temp=>test=>test.bar is found. So shutil.copy will
copy it. I want that once the copy is done, it should make bFound =
True and get out.

What's the use of setting bFound to True ? If you want to get out
returning a value, just return that value :

# dummy exemple
def test(seq, target):
for item in seq:
if item == target:
return True
return False

But since test directory is under temp, work_tree_copy makes two calls
of the same function _but_ break only is able to get out from the inner
call.

You should first focus on making it work without worrying about error
handling. Ensure that all return path are covered (in your actual
implementation, your function always return None). Use a callback
function for testing if a file should be copied, so your code has a
chance of being reusable when specs evolve. And two hints:
* exceptions are not only for error handling, they are also useful to
control flow of execution...
* generators and iterators are great

AFAICT, what you're trying to do here is mainly to
1/ recursively search files matching a given condition in a directory tree
2/ do something with these files.

A solution could be to split your code accordingly:

def find_in_tree(tree_path, match):
join = os.path.join
isdir = os.path.isdir
isfile = os.path.isfile
for name in os.listdir(tree_path):
fpath = join(tree_path, name)
if isdir(fpath):
for foundpath in find_in_tree(fpath, match)
yield foundpath
elif isfile(fpath) and match(fpath):
yield fpath
raise StopIteration

def find_and_copy_to(tree_path,
dest_path, match,
stop_on_first_match=False):
done = []
for fpath in find_in_tree(tree_path, match):
if fpath not in done:
shutil.copy(fpath, dest_path)
done.append(fpath)
if stop_on_first_match:
break
return done

match_foo_bar = lambda fpath: \
fpath.endswith('.foo') or fpath.endswith('.bar')

done = find_and_copy_to(sRepository,
sSourceDir,
match_foo_bar,
stop_on_first_match=True)


NB1 : not tested

NB2: I'm not sure I clearly undestand your specs (you're mixing
functional specs and implementation details in your description of
you're trying to do), so this may not be exactly what you want...

Also some stylistic advices:

1/ hungarian notation is *evil* - and even worse in a dynamically typed
language. Better use explicit names. What's important is not the type of
a variable, but it's role.

2/ string formating is fine

print >> sys.stderr, \
"%s is available in %s - skipping copy" % (filename, source_dir)

Is there a better way to implement it ?

Probably. Anyway, the better implementation is usually no
implementation. What about:

find $Repository -name "*.bar" -exec cp {} $SourceDir/ \;

?-)


Well, time to have a coffee anyway...
HTH

 

[Ritesh Raj Sarraf]

Thanks to everyone. It is really the best place and the best people to
learn from.
Here's what I followed from the discussion:

def files(root):
for path, folders, files in os.walk(root):
for file in files:
yield path, file


def copy_first_match(repository, filename, dest_dir): # aka
walk_tree_copy()
for path, file in files(repository):
if file == filename:
try:
shutil.copy(os.path.join(path, file), dest_dir)
sys.stdout.write("%s copied from local cache %s." %
(file, repository))
except shutil.Error:
sys.stdout.write("%s is available in %s. Skipping
Copy!" % (file, dest_dir))
return True
return False

All I've added is the exception check because in case "file" is
available in "dest_dir", I want to display a message.
Since I'm still new and learning will be great if you let me know if
this is the proper way or not.

Thanks to everyone once again. It's been a very good experience
understanding everyone's comments.

Regards,
rrs

 

[George Sakkis]

Using the (non-standard yet) path module
(http://www.jorendorff.com/articles/python/path/), your code can be
simplified to:

from path import path, copy

def copy_first_match(repository, filename, dest_dir):
try:
first_match = path(repository).walkfiles(filename).next()
except StopIteration:
return False
else:
copy(first_match, dest_dir)
return True


I/O exceptions are left to propagate to the caller.

George

 

[Peter Otten]

def copy_first_match(repository, filename, dest_dir): # aka
walk_tree_copy()
    for path, file in files(repository):
        if file == filename:
            try:
                shutil.copy(os.path.join(path, file), dest_dir)
                sys.stdout.write("%s copied from local cache %s." %
(file, repository))
            except shutil.Error:
                sys.stdout.write("%s is available in %s. Skipping
Copy!" % (file, dest_dir))
            return True
    return False

All I've added is the exception check because in case "file" is
available in "dest_dir", I want to display a message.
Since I'm still new and learning will be great if you let me know if
this is the proper way or not.

I prefer to let the exception propagate. As you wrote it, the code calling
copy_first_match() has no way of knowing whether a file was copied or not.

If you don't want to bother with exceptions in the calling code I'd slightly
modify the try...except:

try:
# copy
except (shutil.Error, OSError), e:
# print failure message
return False

I. e.

- catch OSError, too. I think shutil.copy() is more likely to raise an
OSError than a shutil.Error.
- return False if an exception occurs. That way you can test if something
went wrong like so:

if not copy_first_match(...):
print "file not found or unable to copy it"

Peter