Read and reformat a list of global variables output by a compiler

Question

I'm currently going through a large Python program, with an eye to making it more readable and maintainable. Currently using Pylint and other code review tools throws a lot of unused variable warnings because of the way I've been using .format(). Since this program manipulates a lot of text, I use .format() quite a lot, which results in many spurious warnings. I could turn these warnings off, but I'd rather not because so far they've been useful in finding genuinely unused variables in my code.

Here's an example of the existing code style. (The program is very large so this is just the tip of the iceberg, selected to show the difference in style that I'm considering. Its purpose is to read and process compiler output, which will then be fed to another set of tools. This particular function is from a module which reads and reformats a list of global variables output by a compiler. The names of some other internal functions have been modified a bit to make it easier to read out of context.)

def parse_global_variable(line, length):
    last_element = length-1
    template = ''
    if length == 0:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '\n                0    {width:7}{var_type}'
    else:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '(0..{last_element})\n                0    {width:7}{var_type}'
    module = line.partition('.')[0].rpartition('\t')[2]
    logical_address = line.partition('#')[2].partition('#')[0]
    physical_address = get_physical_address(logical_address)
    var_name = get_var_name(line))
    width = line.rpartition('\t')[2]
    if length != 0:
        width = str(int(width)/length)
    return template.format(**locals())

This is a different style I'm considering, which inlines most of the variables, shortens variable names in the template string, and uses explicit arguments to .format() instead of using **locals():

def parse_global_variable(line, length):
    template = ''
    if length == 0:
        template = ' Record   GLB  {la} {pa} {m:34}{vn}' \
                   '\n                0    {w:7}{vt}'
    else:
        template = ' Record   GLB  {la} {pa} {m:34}{vn}(0..{le})' \
                   '\n                0    {w:7}{vt}'
    logical_address = line.partition('#')[2].partition('#')[0]
    width = line.rpartition('\t')[2]
    if length != 0:
        width = str(int(width)/length)
    return template.format(la=logical_address, pa=get_physical_address(logical_address), 
        m=line.partition('.')[0].rpartition('\t')[2], vn=get_var_name(line), le=length-1, w=width)

Is it stylistically preferable, more readable, more Pythonic, etc. to use **locals() or explicit arguments? Is using abbreviated names such as vn or pa in the format string frowned upon when there are explicit arguments? And should variables which are only used in the format string be inlined?

Edit: Here is a sample input line:

Record      imsd_ini.ini            16#3200#        0       1424

The output that this module produces corresponding to that line is:

Record   GLB  3200 133200 imsd_ini                          ini
              0    1424   ini_type

Answer 1

This is mostly down to personal preference, as both ways are talked about in PEP 498. locals doesn't work well in closures as highlighted in the PEP, and manually writing a=a is verbose. And so both have cons.

PEP 498 was finally added to Python in the latest, stable, version 3.6. And so you can change those to use f-strings. But is not much help here.

---

I personally would choose the verbose way. And if you do, you can combat your problem with your linter:

Currently using Pylint and other code review tools throws a lot of unused variable warnings because of the way I've been using .format().

If they are un-used, then you can define them in the format arguments, and so rather than assigning to a variable that's 'of no use', you can assign straight to the key word argument:

# Current way
>>> a = 1
>>> '{a}'.format(a=a)

# New way
>>> '{a}'.format(a=1)

# When you migrate to Python 3.6 way
>>> a = 1
>>> f'{a}'

Answer 2

The docs for locals() imply that there's some overhead involved with using it:

Update and return a dictionary representing the current local symbol table. Free variables are returned by locals() when it is called in function blocks, but not in class blocks.

So it might be best to avoid it from that perspective alone. (Though it's probably fine in most cases, and avoiding it might be a case of premature optimization.)

I generally prefer verbose format strings when using them this way, using the philosophy of "If the string was separated from the code that uses it, could someone still figure out what each variable in the string is referring to?". So I frequently end up doing somewhat of a hybrid approach -- using a dictionary for the format arguments rather than locals() and rather than having to repeat every single argument as a keyword argument:

def parse_global_variable(line, length):
    last_element = length-1
    template = ''  # This is actually redundant

    f = {
        'module': line.partition('.')[0].rpartition('\t')[2],
        'logical_address': line.partition('#')[2].partition('#')[0],
        'var_name': get_var_name(line),
        'width': line.rpartition('\t')[2]
    }
    f['physical_address'] = get_physical_address(f['logical_address'])

    if length == 0:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '\n                0    {width:7}{var_type}'
    else:
        template = ' Record   GLB  {logical_address} {physical_address} {module:34}{var_name}' \
                   '(0..{last_element})\n                0    {width:7}{var_type}'
        f['width'] = str(int(f['width'])/length)
    return template.format(**f)

Answer 3

Here's a controversial opinion that I'm willing to defend: The spelling x.format(y) is already an antipattern, compared to the straightforward (and shorter) x % y. If you find yourself having to write really long and complicated format strings, that might be a sign that your code could use refactoring.

Your code also has several other lintable problems, such as the dead assignment template = '', a syntactically invalid ) after get_var_name(line), and the fact that you never assign to var_type (so it implicitly refers to the var_type that must exist in your program's global scope).

I would write your given snippet as

def parse_global_variable(line, length):
    global var_type
    module = line.split('.')[0].split('\t')[-1]
    logical_address = line.split('#')[1]
    width = line.split('\t')[-1]
    if length != 0:
        width = str(int(width) / length)  # this calculation is dubious
    return '%7s%6s  %s %s %34s%s%s\n%17d%11s%s' % (
        'Record',
        'GLB',
        logical_address,
        get_physical_address(logical_address),
        module,
        get_var_name(line),
        '(0..%d)' % (length-1) if length != 0 else '',
        0,
        width,
        var_type,
    )

Notice that ' %s %s' would probably better be written as something like '%10s%9s' to maintain column alignment, but I don't know how wide your logical_addresses are expected to be, so I didn't want to just guess. Raw spaces or tabs for column alignment should basically never appear in format strings.

The calculation of variable width is almost certainly wrong, since on one path of the if it receives a string value and on the other path it receives the stringification of an integer value.

I've replaced all the uses of partition with split simply because split is more likely to be understood by the maintainer. "Take the last tab-separated component of x" is more easily written and understood as x.split('\t')[-1] than as x.rpartition('\t')[2]; avoid magic indices whenever possible. Admittedly x.rpartition('\t')[-1] would also work, but split is a much more common verb in Python than rpartition.

Notice that % in Python can also take a dict, so you could write the above as

def parse_global_variable(line, length):
    global var_type
    module = line.split('.')[0].split('\t')[-1]
    logical_address = line.split('#')[1]
    width = line.split('\t')[-1]
    if length != 0:
        width = str(int(width) / length)  # this calculation is dubious
    return '%7(record)s%6(glb)s  %(logical)s %(physical)s %34(module)s%(name)s%(extent)s\n%17(zero)d%11(width)s%(type)s' % {
        'record': 'Record',
        'glb': 'GLB',
        'logical': logical_address,
        'physical': get_physical_address(logical_address),
        'module': module,
        'name': get_var_name(line),
        'extent': '(0..%d)' % (length-1) if length != 0 else '',
        'zero': 0,
        'width': width,
        'type': var_type,
    })

but that's a lot of extra verbiage that obscures the actual workings of the code; personally I would leave it off, and concentrate on reducing the number of individual things I need to print in that single line.