On Mon, Oct 29, 2018 at 3:14 PM Jeff King <peff@xxxxxxxx> wrote:
> Junio's review already covered my biggest question, which is why not
> something like "%(trailers:key=ticket)". And likewise making things like
> comma-separation options.
Jeff, Junio,
thanks!
Your questions pretty much matches what I (and a colleague I discussed
this with before posting) was concerned about.
My first try actually had it as an option to "trailers". But it got a
bit messy with the argument parsing, and the fact that there was a
fast path making it work when only specified. I did not want to spend
lot of time reworking fixing that before I had some feedback, so I
went for a smallest possible patch to float the idea with (a patch is
worth a 1000 words).
I'll start by reworking my patch to handle %(trailers:key=X) (I'll
assume keys never contain ')' or ','), and ignore any formatting until
the way forward there is decided (see below).
> But my second question is whether we want to provide something more
> flexible than the always-parentheses that "%d" provides. That has been a
> problem in the past when people want to format the decoration in some
> other way.
Maybe just like +/-/space can be used directly after %, a () pair can
be allowed.. E.g "%d" would just be an alias for "%()D", and for
trailers it would be something like "%()(trailers:key=foo)"
There is another special cased placeholder %f (sanitized subject line,
suitable for a filename). Which also could be changed to be a format
specifiier, allowing sanitize any thing, e.g "%!an" for sanitized
author name.
Is even the linebreak to commaseparation a generic thing?
"% ,()(trailers:key=Ticket)" it starts go look a bit silly.
Then there are the padding modifiers. %<() %<|(). They operate on next
placeholder. "%<(10)%s" Is that a better syntax?
"%()%(trailers:key=Ticket,comma)"
I can also imagine moving all these modifiers into a generic modifier
syntax in brackets (and keeping old for backwards compat)
%[lpad=10,ltrunc=10]s == %<(10,trunc)%s
%[nonempty-prefix="%n"]GS == %+GS
%[nonempty-prefix=" (",nonempty-suffix=")"]D == %d
Which would mean something like this for tickets thing:
%[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey)
which is kinda verbose.
> We have formatting magic for "if this thing is non-empty, then show this
> prefix" in the for-each-ref formatter, but I'm not sure that we do in
> the commit pretty-printer beyond "% ". I wonder if we could/should add a
> a placeholder for "if this thing is non-empty, put in a space and
> enclose it in parentheses".
Would there be any interest in consolidating those formatters? Even
though they are totally separate beasts today. I think having all
attributes available on long form (e.g "%(authorname)") in addition to
existing short forms in pretty-formatter would make sense.
anders
