Re: [StGit PATCH] Parse commit object header correctly

> 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:
>>> +            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".

>>> +                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.