On Tue, Jun 14, 2011 at 12:15 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> I did a quick review and test of your patch. It didn't quite apply
> cleanly due to recent non-related describe.c changes -- updated patch
> attached.
Thanks for looking at this. Your updated patch looks good to me.
> First, I'll give you a thumbs up on the original inspiration for the
> patch. The output should be standardized, and I see no reason not to
> append a semicolon on usability basis. Beyond that, the changes are
> mostly cosmetic and I can't see how it will break things outside of
> terminating a query early by accident (I didn't see any).
Yeah, I really didn't want to break any queries, so I did my best to
test every query I changed.
> What I do wonder though is if the ; appending should really be
> happening in printQuery() instead of in each query -- the idea being
> that formatting for external consumption should be happening in one
> place. Maybe that's over-thinking it though.
That's a fair point, and hacking up printQuery() would indeed solve my
original gripe about copy-pasting queries out of psql -E. But more
generally, I think that standardizing the style of the code is a Good
Thing, particularly where style issues impact usability (like here). I
would actually like to see a movement towards having all these queries
use whitespace/formatting consistently. For instance, when I do a
\d sometable
I see some queries with lines bleeding out maybe 200 columns, some
wrapped at 80 columns, etc. This sort of style issue is not something
that a simple kludge in printQuery() could solve (and even if we put
in a sophisticated formatting tool inside printQuery(), we'd still be
left with an ugly describe.c). For the record, I find that having SQL
queries consistently formatted makes them much more readable, allowing
a human to roughly "parse" a query on sight once you're used to the
formatting.
So I wouldn't be opposed to putting the kludge in printQuery(), but I
would like to see us standardize the queries regardless. (And in that
case, I wouldn't mind if we dropped all the semicolons in describe.c,
just that we're consistent.)
Josh
