From: | Marko Tiikkaja <marko(at)joh(dot)to> |
---|---|
To: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: new json funcs |
Date: | 2014-01-18 21:05:05 |
Message-ID: | 52DAEC81.9050800@joh.to |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 1/18/14, 9:38 PM, Andrew Dunstan wrote:
> On 01/18/2014 12:34 PM, Marko Tiikkaja wrote:
>> Is it possible for you to generate a diff that doesn't have all these
>> unrelated changes in it (from a pgindent run, I presume)? I just read
>> through three pagefuls and I didn't see any relevant changes, but I'm
>> not sure I want to keep doing that for the rest of the patch.
>>
>
> This seems to be quite overstated. The chunks in the version 3 patch
> that only contain pgindent effects are those at lines 751,771,866 and
> 1808 of json.c, by my reckoning. All the other changes are more than
> formatting.
Oh I see, there's a version 3 which improves things by a lot. I just
took the latest patch from the CF app and that was the v2 patch. Now
looking at it again, I see that it actually added new lines around
json.c:68, which I believe proves my point that reviewing such a patch
is hard.
> And undoing the pgindent changes and then individually applying all but
> those mentioned above would take me a lot of time.
v3 looks "ok". I would have preferred a patch with no unrelated
changes, but I can live with what we have there.
Something like the first three pagefuls of v2, however, would take *me*
a lot of time, which I believe is not acceptable. I don't care why a
patch has lots of unrelated stuff in it, I'm not going to waste my time
trying to figure out which parts are relevant and which aren't. That's
a lot of time wasted just to end up with a review possibly full of
missed problems and misunderstood code.
But I'll continue with my review now that this has been sorted out.
Regards,
Marko Tiikkaja
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2014-01-18 21:09:34 | Re: [PATCH] Make various variables read-only (const) |
Previous Message | Andrew Dunstan | 2014-01-18 20:38:42 | Re: new json funcs |