From: Karthik Nayak <karthik.188@gmail.com>
Date: Sat, 08 Aug 2015 07:03:03 GMT
Subject: Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
Message-ID: <CAOLa=ZT+hCgM+mL2vQknwUjaxG9Mr4C2Qir=aa==Fv9CebYwLg@mail.gmail.com>
In-Reply-To: <CAPig+cTOzZ0RFd2eu43rYQDnsXmfswbRx3=vN41BJf_BHON7tA@mail.gmail.com>

On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which will act as a modifier atom and align
>> any string with or without an %(atom) appearing before a %(end) atom
>> to the right, left or middle.
>>
>> It is followed by `:<type>,<paddinglength>`, where the `<type>` is
>> either left, right or middle and `<paddinglength>` is the total length
>> of the padding to be performed. If the atom length is more than the
>> padding length then no padding is performed. e.g. to pad a succeeding
>> atom to the middle with a total padding size of 40 we can do a
>> --format="%(align:middle,40).."
>>
>> Add documentation and tests for the same.
>
> I forgot to mention in my earlier review of this patch that you should
> explain in the commit message, and probably the documentation, this
> this implementation (assuming I'm understanding the code) does not
> correctly support nested %(foo)...%(end) constructs, where %(foo)
> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or
> some as yet unimagined modifier. Supporting nesting of these
> constructs will require pushing the formatting states onto a stack (or
> invoking the parser recursively).
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Good point, I have been working on this parallely and it works for now,
I'll send that with the %(if) and %(end) feature. But for now, it should be
documented and added in the commit message.

Using a linked list of sorts where whenever a new modifier atom is encountered
a new state is created, and once %(end) is encountered we can pop that state
into the previous state.

-- 
Regards,
Karthik Nayak