Patchwork [2,of,6,RFC] templater: wrap each parsed element with a label filter

login
register
mail settings
Submitter Sean Farley
Date Dec. 24, 2012, 7:03 a.m.
Message ID <75ef781d20973d036f00.1356332589@laptop.local>
Download mbox | patch
Permalink /patch/291/
State Superseded
Headers show

Comments

Sean Farley - Dec. 24, 2012, 7:03 a.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley at gmail.com>
# Date 1356234924 21600
# Node ID 75ef781d20973d036f003bba00a873bf1c5d5598
# Parent  54276c120146e14cc4ab931b799c94bd671ee524
templater: wrap each parsed element with a label filter

By default, this will be a no-op call. If the color extension is enabled then it will be passed an empty label for each element for which it can do something fancy with.
Matt Mackall - Dec. 28, 2012, 9:55 p.m.
On Mon, 2012-12-24 at 01:03 -0600, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley at gmail.com>
> # Date 1356234924 21600
> # Node ID 75ef781d20973d036f003bba00a873bf1c5d5598
> # Parent  54276c120146e14cc4ab931b799c94bd671ee524
> templater: wrap each parsed element with a label filter

I tried to make this clear earlier, but apparently I failed. Let me try
again: any solution that adds complexity to the core of the templater to
enable colorization will be rejected as not-worth-it. This does that,
thus it is rejected. 

Here's a measurement without the first two patches:

$ time hg log --style default -r 0:10000 | wc
  53202  211795 1914499

real	0m2.116s
user	0m2.073s
sys	0m0.033s

And here's one with:

$ time hg log --style default -r 0:10000 | wc
  53202  211795 1914499

real	0m2.500s
user	0m2.450s
sys	0m0.040s

That's about a 20% penalty on all templating (including hgweb!) for a
feature that only even does anything on the command line when color is
enabled and I/O isn't redirected. Not a good trade-off.

Let me also be explicit that the entire class of solutions that try to
track the 'class' of data through the templater are also rejected.
They're too complex, the defaults are semantically wrong anyway, and
it's not-worth-it.

Please send a patch series that:

- adds a no-op label(label, expression) method (note the argument order)
- overrides it in color.py
Sean Farley - Dec. 28, 2012, 10:17 p.m.
On Fri, Dec 28, 2012 at 3:55 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Mon, 2012-12-24 at 01:03 -0600, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley at gmail.com>
>> # Date 1356234924 21600
>> # Node ID 75ef781d20973d036f003bba00a873bf1c5d5598
>> # Parent  54276c120146e14cc4ab931b799c94bd671ee524
>> templater: wrap each parsed element with a label filter
>
> I tried to make this clear earlier, but apparently I failed. Let me try
> again: any solution that adds complexity to the core of the templater to
> enable colorization will be rejected as not-worth-it. This does that,
> thus it is rejected.
>
> Here's a measurement without the first two patches:
>
> $ time hg log --style default -r 0:10000 | wc
>   53202  211795 1914499
>
> real    0m2.116s
> user    0m2.073s
> sys     0m0.033s
>
> And here's one with:
>
> $ time hg log --style default -r 0:10000 | wc
>   53202  211795 1914499
>
> real    0m2.500s
> user    0m2.450s
> sys     0m0.040s
>
> That's about a 20% penalty on all templating (including hgweb!) for a
> feature that only even does anything on the command line when color is
> enabled and I/O isn't redirected. Not a good trade-off.

Ah, thanks for the performance command. I agree that's not a good trade-off.

> Let me also be explicit that the entire class of solutions that try to
> track the 'class' of data through the templater are also rejected.
> They're too complex, the defaults are semantically wrong anyway, and
> it's not-worth-it.

Gotcha.

> Please send a patch series that:
>
> - adds a no-op label(label, expression) method (note the argument order)

Oh, right, I had a question about the argument order. Why is the
expression the second argument? Functions like 'date' and 'fill' take
the expression as the first argument and the date / fill-column as the
second argument.

> - overrides it in color.py

Ok, I can do that. Out of curiosity, how will labeling work in the
GenericTemplating plan?
Matt Mackall - Dec. 28, 2012, 11:39 p.m.
On Fri, 2012-12-28 at 16:17 -0600, Sean Farley wrote:
> On Fri, Dec 28, 2012 at 3:55 PM, Matt Mackall <mpm at selenic.com> wrote:
> > On Mon, 2012-12-24 at 01:03 -0600, Sean Farley wrote:
> >> # HG changeset patch
> >> # User Sean Farley <sean.michael.farley at gmail.com>
> >> # Date 1356234924 21600
> >> # Node ID 75ef781d20973d036f003bba00a873bf1c5d5598
> >> # Parent  54276c120146e14cc4ab931b799c94bd671ee524
> >> templater: wrap each parsed element with a label filter
> >
> > I tried to make this clear earlier, but apparently I failed. Let me try
> > again: any solution that adds complexity to the core of the templater to
> > enable colorization will be rejected as not-worth-it. This does that,
> > thus it is rejected.
> >
> > Here's a measurement without the first two patches:
> >
> > $ time hg log --style default -r 0:10000 | wc
> >   53202  211795 1914499
> >
> > real    0m2.116s
> > user    0m2.073s
> > sys     0m0.033s
> >
> > And here's one with:
> >
> > $ time hg log --style default -r 0:10000 | wc
> >   53202  211795 1914499
> >
> > real    0m2.500s
> > user    0m2.450s
> > sys     0m0.040s
> >
> > That's about a 20% penalty on all templating (including hgweb!) for a
> > feature that only even does anything on the command line when color is
> > enabled and I/O isn't redirected. Not a good trade-off.
> 
> Ah, thanks for the performance command. I agree that's not a good trade-off.
> 
> > Let me also be explicit that the entire class of solutions that try to
> > track the 'class' of data through the templater are also rejected.
> > They're too complex, the defaults are semantically wrong anyway, and
> > it's not-worth-it.
> 
> Gotcha.
> 
> > Please send a patch series that:
> >
> > - adds a no-op label(label, expression) method (note the argument order)
> 
> Oh, right, I had a question about the argument order. Why is the
> expression the second argument? Functions like 'date' and 'fill' take
> the expression as the first argument and the date / fill-column as the
> second argument.

Compare:

label(label, text)
date(date[, format])
fill(text[, column])

We've got conflicting priorities here: default arguments and implied
subject/object/preposition. But I think most people would expect the
label to come first here, reading label(x, y) it as "add label x to text
y", not "label the text x with label type y".

> > - overrides it in color.py
> 
> Ok, I can do that. Out of curiosity, how will labeling work in the
> GenericTemplating plan?

As here: manually.
Sean Farley - Dec. 29, 2012, 12:11 a.m.
On Fri, Dec 28, 2012 at 5:39 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Fri, 2012-12-28 at 16:17 -0600, Sean Farley wrote:
>> On Fri, Dec 28, 2012 at 3:55 PM, Matt Mackall <mpm at selenic.com> wrote:
>> > On Mon, 2012-12-24 at 01:03 -0600, Sean Farley wrote:
>> >> # HG changeset patch
>> >> # User Sean Farley <sean.michael.farley at gmail.com>
>> >> # Date 1356234924 21600
>> >> # Node ID 75ef781d20973d036f003bba00a873bf1c5d5598
>> >> # Parent  54276c120146e14cc4ab931b799c94bd671ee524
>> >> templater: wrap each parsed element with a label filter
>> >
>> > I tried to make this clear earlier, but apparently I failed. Let me try
>> > again: any solution that adds complexity to the core of the templater to
>> > enable colorization will be rejected as not-worth-it. This does that,
>> > thus it is rejected.
>> >
>> > Here's a measurement without the first two patches:
>> >
>> > $ time hg log --style default -r 0:10000 | wc
>> >   53202  211795 1914499
>> >
>> > real    0m2.116s
>> > user    0m2.073s
>> > sys     0m0.033s
>> >
>> > And here's one with:
>> >
>> > $ time hg log --style default -r 0:10000 | wc
>> >   53202  211795 1914499
>> >
>> > real    0m2.500s
>> > user    0m2.450s
>> > sys     0m0.040s
>> >
>> > That's about a 20% penalty on all templating (including hgweb!) for a
>> > feature that only even does anything on the command line when color is
>> > enabled and I/O isn't redirected. Not a good trade-off.
>>
>> Ah, thanks for the performance command. I agree that's not a good trade-off.
>>
>> > Let me also be explicit that the entire class of solutions that try to
>> > track the 'class' of data through the templater are also rejected.
>> > They're too complex, the defaults are semantically wrong anyway, and
>> > it's not-worth-it.
>>
>> Gotcha.
>>
>> > Please send a patch series that:
>> >
>> > - adds a no-op label(label, expression) method (note the argument order)
>>
>> Oh, right, I had a question about the argument order. Why is the
>> expression the second argument? Functions like 'date' and 'fill' take
>> the expression as the first argument and the date / fill-column as the
>> second argument.
>
> Compare:
>
> label(label, text)
> date(date[, format])
> fill(text[, column])
>
> We've got conflicting priorities here: default arguments and implied
> subject/object/preposition. But I think most people would expect the
> label to come first here, reading label(x, y) it as "add label x to text
> y", not "label the text x with label type y".

Ah, ok, that makes more sense.

>> > - overrides it in color.py
>>
>> Ok, I can do that. Out of curiosity, how will labeling work in the
>> GenericTemplating plan?
>
> As here: manually.

Well, a potential downside to this that I immediately see is not
having a way to get access to the current context's data from the
command line, e.g. the phase. Something hackish could be done in the
form of:

if 'log.changeset' in label:
  label += ' changeset.' + ctx.phasestr()

but, at most, this is just a stopgap.

Unfortunately, changing the color based on the phase is the only
feature I really want out of this whole template-labeling business
({files} would be nice but since {file_mods/dels/adds} exist, one can
work around not having access to the current context). I'm open to
options (and can certainly send the two patches that get most of the
work done) but in the end is seems my effort to learn the ins-and-outs
of the templating engine was all in vain.
Matt Mackall - Dec. 29, 2012, 12:23 a.m.
On Fri, 2012-12-28 at 18:11 -0600, Sean Farley wrote:
> >> Ok, I can do that. Out of curiosity, how will labeling work in the
> >> GenericTemplating plan?
> >
> > As here: manually.
> 
> Well, a potential downside to this that I immediately see is not
> having a way to get access to the current context's data from the
> command line, e.g. the phase. Something hackish could be done in the
> form of:

> if 'log.changeset' in label:
>   label += ' changeset.' + ctx.phasestr()

What about:

label('changeset.{phase}', expr)
Sean Farley - Dec. 29, 2012, 12:32 a.m.
On Fri, Dec 28, 2012 at 6:23 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Fri, 2012-12-28 at 18:11 -0600, Sean Farley wrote:
>> >> Ok, I can do that. Out of curiosity, how will labeling work in the
>> >> GenericTemplating plan?
>> >
>> > As here: manually.
>>
>> Well, a potential downside to this that I immediately see is not
>> having a way to get access to the current context's data from the
>> command line, e.g. the phase. Something hackish could be done in the
>> form of:
>
>> if 'log.changeset' in label:
>>   label += ' changeset.' + ctx.phasestr()
>
> What about:
>
> label('changeset.{phase}', expr)

Because ... I am an idiot.

Patch

diff --git a/mercurial/templater.py b/mercurial/templater.py
--- a/mercurial/templater.py
+++ b/mercurial/templater.py
@@ -95,10 +95,12 @@ 
 
         pd = [tmpl, n + 1, stop]
         parseres, pos = p.parse(pd)
         parsed.append(parseres)
 
+    parsed = [('func', ('symbol', 'label'), e) for e in parsed]
+
     return [compileexp(e, context) for e in parsed]
 
 def compileexp(exp, context):
     t = exp[0]
     if t in methods: