2

I've recently decided to start using .format() instead of % (see this question). Instead of the {0}, {1} syntax, I'm wondering if the following is an acceptable use:

import os
def get_filename(player_name):
    for ext in ('jpg', 'jpeg', 'png'):
        filename = "data/avatars/{player_name}.{ext}".format(**locals())
        if os.path.exists(filename):
            return filename
    return None

I like the straightforwardness of it - local variables go into the string - but am wondering if there's any reason I shouldn't do the above.

Community
  • 1
  • 1
Claudiu
  • 224,032
  • 165
  • 485
  • 680
  • You may not want to expose your variables if the string came from untrusted sources. But, the way it is, there's no problem. – JBernardo Mar 28 '13 at 19:14
  • If you're ever, even possibly, going to be formatting strings submitted by untrusted sources, you definitely do not want to do this. Beyond that, some pythonistas dislike this as being "implicit rather than explicit"—there are debates on the mailing lists every time someone suggests that the language should make this easier/more obvious—but others disagree, so I'd say it's a style issue that hasn't been settled. – abarnert Mar 28 '13 at 19:15
  • I agree with the point about exposing locals to potentially-untrusted sources. That said, I personally don't have a problem with using `**` in this style. – Dan Lecocq Mar 28 '13 at 19:20
  • @abarnert: Yea I was just thinking it would be nice to have the default of `.format()` use the locals dict. or maybe a separate function, `.format_locals()`, to make it more explicit. definitely wouldn't want magic behavior for `{}` in strings without calling a function on it, though. – Claudiu Mar 28 '13 at 19:20
  • Good point about the untrusted strings, though that would apply with `%` also. Out of curiousity, what's the worst a malicious string could do in this situation? The calling code takes the filename and loads it using `pygame.image.load`. – Claudiu Mar 28 '13 at 19:21
  • You can't make it into a function unless you go to the frame above to get the right `locals`. This is usually not a nice design – JBernardo Mar 28 '13 at 19:22
  • @JBernardo: oh right, that's a good point even if it were a built-in function. that would be sort of magical. then maybe a syntax extension like `@"data/avatars{player_name}.{ext}"`... but then that's changing the language quite a bit – Claudiu Mar 28 '13 at 19:23
  • 1
    This might confuse some tools working on python code, eg. spurious warnings like variable `ext` is unused in this function. – zch Mar 28 '13 at 19:25
  • Well, `"{foo.bar}".format(**locals())` calls `__getattr__`. That might be bad. – Katriel Mar 28 '13 at 19:25
  • 1
    It's no better or worse than with `%`. The arguments about making it easier or harder to magically `%`-format with locals just turned into arguments about making it easier or harder to magically `{}`-format with locals, with no changes to the arguments besides the syntax. – abarnert Mar 28 '13 at 19:26
  • @abarnert: hah, that's pretty funny. – Claudiu Mar 28 '13 at 19:26
  • And the main thing a malicious string can do is get you to expose some local variable like, say, `master_db_password`. – abarnert Mar 28 '13 at 19:26

2 Answers2

4

The major problem with passing locals() (or globals()) to format (or %) is that often, format strings can come from untrusted sources, and you risk exposing variables you didn't want to. If you're just formatting a literal string, that isn't an issue, but if you ever may have untrusted format strings, you have to think very carefully about what you're doing—and it's easier to just not do it.

The more minor problem is that some of your code's readers won't understand locals, or the ** syntax, and will have a hard time figuring out what it does or why it works. This isn't much of an argument. In fact, you could even say that a lot of Python's design decisions come down to making sure this is almost never a good argument—the language is exactly big enough that it's reasonable to expect your readers to understand/learn anything pythonic you write. But it's still worth thinking about.

Then there's the style issue. Every few months, someone comes along suggesting that the language should make it easier to do this, and it starts an argument on the mailing lists. Some people definitely think that this feels "implicit rather than explicit". Others disagree. I think it's pretty well settled that magic locals here would not be pythonic… but if you have to explicitly pass locals(), maybe that's fine and maybe it isn't. Like most style arguments that haven't gathered a consensus, it's really up to you. (By the way, the format API ultimately came out of an argument like this, where the original suggestion was for more-perl-like string interpolation with an implicit locals.)

But ultimately, you have to consider what you're saving. Compare this:

filename = "data/avatars/{player_name}.{ext}".format(**locals())

to:

filename = "data/avatars/{0}.{1}".format(player_name, ext)

Your version isn't clearer, more explicit, easier to type, or even shorter. So, I'd say the risk of making it a little harder for novices to read, and annoying to some segment of the community (even if it's for bad reasons), isn't worth it if there's no benefit.

abarnert
  • 354,177
  • 51
  • 601
  • 671
  • 1
    One might argue that with a long format string with a lot of placeholders it's hard to track which placeholder is which argument (one has to count), and with explicit keyword arguments like `"{player_name}".format(player_name=player_name)` there's some smelly triplication. – Pavel Anossov Mar 28 '13 at 19:32
  • @PavelAnossov: yea that was my main issue. I like having the format string explicit, but then the parameters to `.format` are not pretty, unless i do the `**locals()` which is arguably hack-y. I think abarnert is right, though, that in this particular case they're both equally readable, if not the latter being clearer, and to be honest I haven't had a situation where I have had trouble keeping track of the format chars yet. – Claudiu Mar 28 '13 at 19:42
  • @PavelAnossov: True. Then again, you could argue that a long format string with lots of placeholders should be using some more sophisticated templating than `format`. – abarnert Mar 28 '13 at 19:43
1

As I commented above, this should not be used on untrusted sources. Also it may not me explicit enough to be Pythonic.

One can also define a function to do that, but to access the right locals, it needs to do some frame handling

def format_locals(string):
    return string.format(**sys._getframe().f_back.f_locals)

This kind of pattern is not nice and things like Pypy can't optmize this kind of code.

I would use this code (unless you need Python 2.6 support so you must add the indexes):

filename = 'data/avatars/{}.{}'.format(player_name, ext)
JBernardo
  • 32,262
  • 10
  • 90
  • 115
  • You might want to add the indexes even for 2.7/3.x, if you're doing the too-common "build it for US_en now, figure out how to internationalize later" thing. – abarnert Mar 28 '13 at 19:42