Unexpected NameError

Question

I'm writing a script to batch-rename all files inside a folder. I'm trying to make it modular, so the core algorithm (the one that generates the new file name) is easily exchangeable.

Here's what I've got so far:

from os import listdir, rename

def renamer(path, algorithm, counter=False, data=None, data2=None, safe=True):

    call_string = 'new=algorithm(i'
    if counter:
        call_string += ', file_index'
    if data != None:
        call_string += ', data'
    if data2 != None:
        call_string += ', data2'
    call_string += ')'

    if safe:
        print('Press Enter to accept changes. '+
        'Type anything to jump to the next file.\n')

    files_list = listdir(path)
    for i in files_list:
        file_index = files_list.index(i)
        old = i
        exec(call_string)
        if safe:
            check = input('\nOld:\n'+old+'\nNew:\n'+new+'\n\nCheck?\n\n')
            if check is not '':
                continue
        rename(path+old, path+new)

    return

Now for some reason (seems unexplicable to me), calling the function raises NameError:

>>> def f(s):
    return 'S08'+s

>>> path='C:\\Users\\****\\test\\'
>>> renamer(path, f)
Press Enter to accept changes. Type anything to jump to the next file.

Traceback (most recent call last):
  File "<pyshell#39>", line 1, in <module>
    renamer(path, f)
  File "C:\Python32\renamer.py", line 25, in renamer
    check = input('\nOld:\n'+old+'\nNew:\n'+new+'\n\nCheck?\n\n')
NameError: global name 'new' is not defined

Unexplicable because, at line 25 it should already have executed call_string, thereby defining the name new. I've been trying to figure out my mistake for over an hour now, I've went through the entire code entering line for line into the shell twice, and it worked fine, and I can't seem to figure out the problem.

Can somebody please help me figure out where I went wrong?

Edit: I've already guessed it might be possible you can't assign names using exec, so I've tested it as follows, and it worked:

>>> exec('cat="test"')
>>> cat
'test'

Ugh, ugh, ugh!!!! Don't use `exec` like this! Just let people write their own name-generating _functions_ and pass those around! — Katriel, Jul 18 '11 at 12:19

gurney alex · Accepted Answer · 2011-07-18T13:15:37.667

3

don't use exec or eval for this, and just write

new = algorithm(i, file_index, data, data2)

make sure all your algorithms can use the 4 arguments (ignoring those they don't need).

If you don't like this, the following is much more pythonic and efficient than using eval:

args = [i] 
if counter:
    args.append(file_index)
for arg in (data, data2):
    if arg is not None:
        args.append(arg)

new = algorithm(*args)

Also replace the ugly:

for i in files_list:
    file_index = files_list.index(i)

with

for index, filename in enumerate(file_list):
    ...

finally, use os.path.join to concatenate paths parts instead of string concatenation. This will save you debugging time when you call the function with a directory name without a trailing '/'

edited Jul 18 '11 at 13:15

answered Jul 18 '11 at 12:13

gurney alex

13,247
4
43
57

thx, nice tips. I'll be leaving it with calling the algorithm through a string, though, since I find defining functions with arguments they don't need disturbing ^^ – Axim Jul 18 '11 at 12:24
1

@Axim: I insist, you really want to limit your use of eval and exec in your python code. I've added a way of avoiding it which enables you not to add unnecessary arguments to your algorithms. – gurney alex Jul 18 '11 at 12:40
2

@Axim: While `exec` and `eval` have [a few] legit uses this is positively _not_ one of them! A rule of thumb is to consider exec/eval a `code smell` _a priori_, and only to allow these after a earnest effort at proving yourself -and others- this not the case. – mjv Jul 18 '11 at 12:46
ok that seems like a better way indeed. I initially went with exec because I didn't know there's a way around it. – Axim Jul 18 '11 at 13:09

Björn Pollex · Answer 2 · 2011-07-18T12:29:29.717

1

You declare the name new inside the exec-call. It is not visible outside. That is why the the error is generated when you try to access new after the call to exec, and not inside exec.

I do not see any reason why you would use exec here in the first place. The way you build call_string, you could just call algorithm directly.

If you really want your algorithms to be able to take variable arguments, use keyword arguments.

edited Jul 18 '11 at 12:29

answered Jul 18 '11 at 12:07

Björn Pollex

75,346
28
201
283

The way i build call_string is because sometimes, I want them to be able to use up to 4 arguments, while sometimes (as in the example) I just want them to use one. – Axim Jul 18 '11 at 12:20
You can just require (this should be written in your documentation), that `algorithm` should be able to take between 1 and 4 arguments. – Björn Pollex Jul 18 '11 at 12:23

Ned Batchelder · Answer 3 · 2011-07-18T12:45:31.807

You don't need exec. You can adjust the arguments passed to the function so that it adapts:

def renamer(path, algorithm, counter=False, data=None, data2=None, safe=True):

    if safe:
        print('Press Enter to accept changes. '+
        'Type anything to jump to the next file.\n')

    files_list = listdir(path)
    for file_index, old in enumerate(files_list):
        opt_args = []
        if counter:
            opt_args.append(file_index)
        if data is not None:
            opt_args.append(data)
        if data2 is not None:
            opt_args.append(data2)
        new = algorithm(old, *opt_args)
        if safe:
            check = input('\nOld:\n'+old+'\nNew:\n'+new+'\n\nCheck?\n\n')
            if check:
                continue
        rename(path+old, path+new)

A few other minor points: use "is not None" instead of "!= None"; to see if a string is empty, just use "if check"; and you don't need a bare return at the end of a function. I've also included the enumerate improvement suggested by @gurney alex.

score 0 · Answer 4 · answered Jul 18 '11 at 12:09

0

Change call_string = 'new=algorithm(i' to call_string = 'algorithm(i' and change exec(call_string) to new = eval(call_string) and you shouldn't have any problems.

answered Jul 18 '11 at 12:09

agf

171,228
44
289
238

1

@Axim: While that may have worked great, note that `exec` and `eval` are [almost never the appropriate choice](http://stackoverflow.com/questions/1832940/). If you want to improve your skills as a Python programmer, don't use it here. Please. – Björn Pollex Jul 18 '11 at 12:26
@Space_C0wb0y A number of core Python developers have also commented that they don't think there is any problem with exec and eval when used appropriately. Also here's another stackoverflow reference for you, http://stackoverflow.com/questions/4158117/use-of-exec-and-eval-in-python – agf Jul 18 '11 at 12:37
@Space_C0wb0y To be clear, yes, it's obvious you don't need exec or eval here. But I answered the question -- which "don't use exec or eval" doesn't. – agf Jul 18 '11 at 12:40
@agf: stack overflow is not only to "answer questions", it is to enhance everyone's coding ability and serve as future reference. Pointing out the "you're doing it wrong" bits is part of answering questions here. – gurney alex Jul 18 '11 at 12:52
@gurney-alex Yes, I agree those are absolutely useful comments and answers, I just wanted to point out my answer wasn't wrong, just of narrower scope. – agf Jul 18 '11 at 13:02

Unexpected NameError

4 Answers4