Re: [StGit PATCH] Parse commit object header correctly

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/08/2012 11:43 AM, Frans Klaver wrote:
> 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".

Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension.  If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.

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

I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/