Re: [StGit PATCH] Parse commit object header correctly

From: Frans Klaver <fransklaver@gmail.com>
Date: Feb 8, 2012, 10:43 UTC
Message-ID: <CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com>
In-Reply-To: <4F3247CA.1020904@alum.mit.edu>

On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>
>>  (1) detects end of the hedaer correctly by treating only an empty line as
>>      such;

s/hedaer/header/;

Show 18 quoted lines

>> +            line = lines[i].rstrip('\n')
>> +            ix = line.find(' ')
>> +            if 0 <= ix:
>> +                key, value = line[0:ix], line[ix+1:]
>
> The above five lines can be written
>
>           for line in lines:
>               if ' ' in line:
>                   key, value = line.rstrip('\n').split(' ', 1)
>
> or (if the lack of a space should be treated more like an exception)
>
>           for line in lines:
>               try:
>                   key, value = line.rstrip('\n').split(' ', 1)
>               except ValueError:
>                   continue

This is generally considered more pythonic: "It's easier to ask for forgiveness than to get permission".

Show 40 quoted lines

>
>> +                if key == 'tree':
>> +                    cd = cd.set_tree(repository.get_tree(value))
>> +                elif key == 'parent':
>> +                    cd = cd.add_parent(repository.get_commit(value))
>> +                elif key == 'author':
>> +                    cd = cd.set_author(Person.parse(value))
>> +                elif key == 'committer':
>> +                    cd = cd.set_committer(Person.parse(value))
>
> All in all, I would recommend something like (untested):
>
>        @return: A new L{CommitData} object
>        @rtype: L{CommitData}"""
>        cd = cls(parents = [])
>        lines = []
>        raw_lines = s.split('\n')
>        # Collapse multi-line header lines
>        for i, line in enumerate(raw_lines):
>            if not line:
>                cd.set_message('\n'.join(raw_lines[i+1:]))
>                break
>            if line.startswith(' '):
>                # continuation line
>                lines[-1] += '\n' + line[1:]
>            else:
>                lines.append(line)
>
>        for line in lines:
>            if ' ' in line:
>                key, value = line.split(' ', 1)
>                if key == 'tree':
>                    cd = cd.set_tree(repository.get_tree(value))
>                elif key == 'parent':
>                    cd = cd.add_parent(repository.get_commit(value))
>                elif key == 'author':
>                    cd = cd.set_author(Person.parse(value))
>                elif key == 'committer':
>                    cd = cd.set_committer(Person.parse(value))
>        return cd

One could also take the recommended python approach for switch/case-like if/elif/else statements:

updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
                 'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
                 'author': lambda cd, value: cd.set_author(Person.parse(value)),
                 'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
              }
for line in lines:
    try:
        key, value = line.split(' ', 1)
        cd = updater[key](cd, value)
    except ValueError:
        continue
    except KeyError:
        continue

It documents about the same, but adds checking on double 'case' statements. The resulting for loop is rather cleaner and the exception approach becomes even more logical. I rather like the result, but I guess it's mostly a matter of taste.

Cheers, Frans