From: Frans Klaver <fransklaver@gmail.com>
Date: Wed, 08 Feb 2012 20:04:16 GMT
Subject: Re: [StGit PATCH] Parse commit object header correctly
Message-ID: <op.v9dl1e0v0aolir@keputer.lokaal>
In-Reply-To: <4F32A014.1000608@alum.mit.edu>

On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty  
<mhagger@alum.mit.edu> wrote:

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

I find the documenting more convincing than the efficiency, but from the  
phrasing I think you do too.


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

Well, as I said, its largely a matter of taste; four items is a corner  
case to me when thinking maintainability vs. readability. On the other  
hand, this doesn't seem like an oft-changing piece of code, so a longer  
list of if..elif..else shouldn't be a problem either.

Frans