[PATCH 5 of 8] mq: make patchheader .plainmode more reliable

[Pierre-Yves David]

On 09/24/2014 04:54 PM, Mads Kiilerich wrote:
> On 09/24/2014 03:35 AM, Pierre-Yves David wrote:
>>
>>
>> On 09/23/2014 06:00 PM, Mads Kiilerich wrote:
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski at unity3d.com>
>>> # Date 1411225616 -7200
>>> #      Sat Sep 20 17:06:56 2014 +0200
>>> # Node ID 39cc9d328c6e61f631ae4f96b5bb0702f3145d39
>>> # Parent  889f7da574226e636d689fa4dd425b2b1fbb5ac5
>>> mq: make patchheader .plainmode more reliable
>>>
>>> Instead of having to make extra checks whenever we use .plainmode,
>>> let the
>>> initial value consider the actual patch header content.
>>>
>>> diff --git a/hgext/mq.py b/hgext/mq.py
>>> --- a/hgext/mq.py
>>> +++ b/hgext/mq.py
>>> @@ -202,7 +202,11 @@ class patchheader(object):
>>>           self.nodeid = nodeid
>>>           self.branch = branch
>>>           self.haspatch = diffstart > 1
>>> -        self.plainmode = plainmode
>>> +        self.plainmode = (plainmode or
>>> +                          '# HG changeset patch' not in
>>> self.comments and
>>> +                          util.any(c.startswith('Date: ') or
>>> +                                   c.startswith('From: ')
>>> +                                   for c in self.comments))
>>
>> 5 lines long "oneliner"‽ Can you use temporary variable to get this
>> readable ?
>>
>
> Yes, it is an expression spanning 5 lines. What is the problem?

Because it is harder to read than plain code. Why does it matter to be 
in a single line?

> Plainmode is when the plainmode parameters says so or when it not is a
> HG patch and it has Date or From fields. Exactly what the expression says.
>
> It could of course be written differently and more verbose and more
> inefficient to read and execute.
>
> Would you prefer a more imperative 6-liner that is more verbose in
> explaining "how" but hides the "why"?
>
>          self.plainmode = plainmode
>          if not plainmode:
>              if '# HG changeset patch' not in self.comments:
>                  self.plainmode = util.any(c.startswith('Date: ') or
>                                            c.startswith('From: ')
>                                            for c in self.comments)

This is easier to read. The two first conditionals can probably be merge 
into one as it will remains a single line.

   if not plainmode and '# HG changeset patch' not in self.comment:

Then the use of any does not help the compactness here we could also 
expand it into an explicit for loop:

   for c in self.comments:
       if c.startswith('Date: ') or c.startswith('From: '):
           self.plainmode = True:
	  break

Also X in self.comments is also doing an extra traversal of 
self.comment. But I believe this is not very important at this list will 
remains small.

> What could be put in temporary variables? Unconditionally looping
> through the comments 3 times would be so obviously inefficient that it
> would be hard to look through that and read what happens.
>
> (/me very much dislikes the pattern we use in some places where we
> introduce a redundant variable just to avoid wrapping a line.)

My main complain (but no only complains) is that the flow of "and", "or" 
and "()" are hard to read when mixed in a multiple lines soup. I much 
prefers when it has the same look as any other conditional.

> /Mads
> not just being polemic but really not seeing how it could be made more
> readable

I prefers multiple line stuff. But is you strongly disagree, you can 
growl a bit louder and I'll let you turn the MQ code a bit more unreable ;-)

-- 
Pierre-Yves David