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

Mads Kiilerich mads at kiilerich.com
Thu Sep 25 11:06:49 CDT 2014


On 09/25/2014 02:27 AM, Pierre-Yves David wrote:
> 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?

I do not agree that it is harder to read. I think math is a fine 
abstraction and I like to leave it to compilers/interpreters to 
translate that to the more low level imperative commands that computers 
can execute. It is also easy for us to follow imperative code but it is 
hard to deduce the high level concepts from that.

Conclusion: We have different preferences.

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

Is it really the number of lines that matters? Would it be fine if it 
used single letter variable names so everything could fit on 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

Specifying in detail how to compute it is one kind of explicit. The bad 
kind, IMO.

I think that using util.any when that is what I want is more explicit. I 
think list/set/dict comprehensions are a simple and powerful notation 
that we should use when applicable.

The reason I prefer any() is not because it is compact but because it is 
a good and readable abstraction.

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

Yes, the data model is not optimized for efficiency. That is not the 
topic of this patch.

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

And I think that a simple expression with an 'and' and an 'or' is easy 
to read - especially when the whitespace makes it easy to see what the 
operands are.

I do not see it as a conditional - I see it as an expression that 
abstracts the conditionals ... but at the same time allows me to zoom in 
on how boolean short-circuit evaluation makes it efficient.

>
>> /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 ;-)

I guess all the words I use here can be considered loud growling ;-)

What you are saying is not completely wrong but I do not see it as 
applicable here. It is not a matter of violating the coding style or 
pushing the limits of what is acceptable. It seems more like you in this 
case are trying to make others follow your personal subjective opinion 
on minor issues in a way that (even if you were right) doesn't improve 
the code or help the project.

I will of course comply if everybody else disagree or if I am violating 
an explicit (or too obvious and trivial to state explicitly) coding style.

/Mads



More information about the Mercurial-devel mailing list