On Sat, Feb 15, 2014 at 11:08 AM, Emre Hasegeli <emre@hasegeli.com> wrote:
> This is my review about 3th version of the patch. It is an useful
> improvement in my opinion. It worked well on my environment.
I'm reviewing this patch.
One thing to comment:
With no doc changes and no regression tests I was halfway inclined to
just reject it out of hand. To be fair there were no regression tests
for wrapped output prior to the patch but still I would have wanted to
see them added. We often pare down regression tests when committing
patches but it's easier to pare them down than write new ones and it
helps show the author's intention.
In this case I'm inclined to expand the regression tests. We've had
bugs in these formatting functions before and at least I find it hard
to touch code like this with any confidence that I'm not breaking
things. Formatting code ends up being pretty spaghetti-like easily and
there are lots of cases so it's easy to unintentionally break cases
you didn't realize you were touching.
In addition there are several cases of logic that looks like this:
if (x) ..
else { if (y) ... else ... }
I know there are other opinions on this but I find this logic very
difficut to follow. It's almost always clearer to refactor the
branches into a flat if / else if / else if /.../ else form. Even if
it results in some duplication of code (typically there's some trivial
bit of code outside the second "if") it's easy to quickly see whether
all the cases are handled and understand whether any of the cases have
forgotten any steps.
--
greg