Patchwork [2,of,6] branches: format rev as integer that is necessary for generic templater

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 2, 2014, 2:59 p.m.
Message ID <04e13b88d3b047406ca9.1412261944@mimosa>
Download mbox | patch
Permalink /patch/6084/
State Superseded
Commit 08a4e70ed1839788ce523fcfd93ad3c4c4c95d1d
Headers show

Comments

Yuya Nishihara - Oct. 2, 2014, 2:59 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1412255739 -32400
#      Thu Oct 02 22:15:39 2014 +0900
# Node ID 04e13b88d3b047406ca954fbb79de228874817eb
# Parent  4ec9dd1b52889c73bbf39159222a161e4374cd4a
branches: format rev as integer that is necessary for generic templater
Mads Kiilerich - Oct. 2, 2014, 3:23 p.m.
On 10/02/2014 04:59 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1412255739 -32400
> #      Thu Oct 02 22:15:39 2014 +0900
> # Node ID 04e13b88d3b047406ca954fbb79de228874817eb
> # Parent  4ec9dd1b52889c73bbf39159222a161e4374cd4a
> branches: format rev as integer that is necessary for generic templater
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1119,8 +1119,10 @@ def branches(ui, repo, active=False, clo
>               notice = _(' (inactive)')
>           if tag == repo.dirstate.branch():
>               label = 'branches.current'
> -        rev = str(ctx.rev()).rjust(31 - encoding.colwidth(tag))
> -        rev = ui.label('%s:%s' % (rev, hexfunc(ctx.node())),
> +        rev = ctx.rev()
> +        fmt = ' ' * max(31 - len(str(rev)) - encoding.colwidth(tag), 0)
> +        fmt += '%d:%s'

I think it is sad that our 80 characters line limit cause us to spend 
extra cpu cycles. It will not make a real difference here but it just 
feels wrong. I have a hard time trying to understand how this can be 
more readable than a multi line statement.

/Mads

> +        rev = ui.label(fmt % (rev, hexfunc(ctx.node())),
>                          'log.changeset changeset.%s' % ctx.phasestr())
>           labeledtag = ui.label(tag, label)
>           if ui.quiet:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 2, 2014, 3:50 p.m.
On Thu, 02 Oct 2014 17:23:32 +0200, Mads Kiilerich wrote:
> On 10/02/2014 04:59 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1412255739 -32400
> > #      Thu Oct 02 22:15:39 2014 +0900
> > # Node ID 04e13b88d3b047406ca954fbb79de228874817eb
> > # Parent  4ec9dd1b52889c73bbf39159222a161e4374cd4a
> > branches: format rev as integer that is necessary for generic templater
> >
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -1119,8 +1119,10 @@ def branches(ui, repo, active=False, clo
> >               notice = _(' (inactive)')
> >           if tag == repo.dirstate.branch():
> >               label = 'branches.current'
> > -        rev = str(ctx.rev()).rjust(31 - encoding.colwidth(tag))
> > -        rev = ui.label('%s:%s' % (rev, hexfunc(ctx.node())),
> > +        rev = ctx.rev()
> > +        fmt = ' ' * max(31 - len(str(rev)) - encoding.colwidth(tag), 0)
> > +        fmt += '%d:%s'
> 
> I think it is sad that our 80 characters line limit cause us to spend 
> extra cpu cycles. It will not make a real difference here but it just 
> feels wrong. I have a hard time trying to understand how this can be 
> more readable than a multi line statement.

Ok, I'll do

  padsize = ...
  fmt = ' ' * padsize + '%d:%s'

Regards,
Mads Kiilerich - Oct. 2, 2014, 6:19 p.m.
On 10/02/2014 05:50 PM, Yuya Nishihara wrote:
> On Thu, 02 Oct 2014 17:23:32 +0200, Mads Kiilerich wrote:
>> On 10/02/2014 04:59 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1412255739 -32400
>>> #      Thu Oct 02 22:15:39 2014 +0900
>>> # Node ID 04e13b88d3b047406ca954fbb79de228874817eb
>>> # Parent  4ec9dd1b52889c73bbf39159222a161e4374cd4a
>>> branches: format rev as integer that is necessary for generic templater
>>>
>>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>>> --- a/mercurial/commands.py
>>> +++ b/mercurial/commands.py
>>> @@ -1119,8 +1119,10 @@ def branches(ui, repo, active=False, clo
>>>                notice = _(' (inactive)')
>>>            if tag == repo.dirstate.branch():
>>>                label = 'branches.current'
>>> -        rev = str(ctx.rev()).rjust(31 - encoding.colwidth(tag))
>>> -        rev = ui.label('%s:%s' % (rev, hexfunc(ctx.node())),
>>> +        rev = ctx.rev()
>>> +        fmt = ' ' * max(31 - len(str(rev)) - encoding.colwidth(tag), 0)
>>> +        fmt += '%d:%s'
>> I think it is sad that our 80 characters line limit cause us to spend
>> extra cpu cycles. It will not make a real difference here but it just
>> feels wrong. I have a hard time trying to understand how this can be
>> more readable than a multi line statement.
> Ok, I'll do
>
>    padsize = ...
>    fmt = ' ' * padsize + '%d:%s'

Grumpy me also dislikes introducing extra name bindings just to get 
short lines. In this case the padsize is a welldefined concept and 
naming it serves as documentation, so in this case it isn't that bad.

Others might however love both this and your initial proposal. Pleasing 
everybody will be hard. Do what you think is right and fight for it ;-)

/Mads

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1119,8 +1119,10 @@  def branches(ui, repo, active=False, clo
             notice = _(' (inactive)')
         if tag == repo.dirstate.branch():
             label = 'branches.current'
-        rev = str(ctx.rev()).rjust(31 - encoding.colwidth(tag))
-        rev = ui.label('%s:%s' % (rev, hexfunc(ctx.node())),
+        rev = ctx.rev()
+        fmt = ' ' * max(31 - len(str(rev)) - encoding.colwidth(tag), 0)
+        fmt += '%d:%s'
+        rev = ui.label(fmt % (rev, hexfunc(ctx.node())),
                        'log.changeset changeset.%s' % ctx.phasestr())
         labeledtag = ui.label(tag, label)
         if ui.quiet: