Re: [PATCH v9 03/11] ref-filter: implement an `align` atom

[Karthik Nayak <karthik.188@gmail.com>]

On Sun, Aug 9, 2015 at 1:30 PM, Matthieu Moy <matthieu.moy@imag.fr> wrote:
>
>
> Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak <karthik.188@gmail.com> a écrit :
>>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.
>
> Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait.
>
> Cheers,
>

Agreed, but it was just something I had already worked on. Probably
will push that series after GSoC :)


-- 
Regards,
Karthik Nayak