[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