Re: [PATCH v7 03/11] ref-filter: add option to pad atoms to the right
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Aug 1, 2015, 06:03 UTC
- Message-ID
- <CAOLa=ZSXM3CmtVvVFRvQ340fKAKPHvPo64r41CSvoXrFz+wm9w@mail.gmail.com>
- In-Reply-To
- <CAPig+cR709U7zy3+F4eoiyUBhA=c2ZCxPc9uUoDQLOuooFxvzQ@mail.gmail.com>
On Thu, Jul 30, 2015 at 11:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 18 quoted lines
> > In an earlier review, Matthieu pointed out that this test failed to > ensure that the 'padright' value did not leak into the next atom. In a > subsequent version, you fixed the test to check that condition, but > now you've somewhat lost it again, at least visually. That is, because > whitespace is "invisible" and because 'padright' now also affects > literal strings, someone reading this test can't tell if those > trailing |'s in the expected output are padded or not. You could use a > different format string to prove that the 'padright' value doesn't > leak. For instance: > > %(padright:10)%(refname:short)|%(padright:5)|%(refname:short) > > This way, as long as the two |'s are side-by-side, then you've proved > that the first one wasn't affected by the preceding 'padright:10'. You > could also add back the %(refname:short) at the front of the pattern, > as you currently have it, if you want to prove that padding is not > enabled at the start of format.
Thanks Eric. This seems like a better improvement over my test. Will implement this :)
-- Regards, Karthik Nayak