review: psql: edit function, show function commands patch - Mailing list pgsql-hackers

Hi,

here's a review of the \sf and \ef [num] patch from
http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19de29@mail.gmail.com

== Formatting ==

The patch has some small tabs/spaces and whitespace  issues and it
applies with some offsets, I ran pgindent and rebased against HEAD,
attaching the resulting patch for your convenience.

== Functionality ==

The patch adds the following features:
  * \e file.txt num  ->  starts a editor for the current query buffer
and puts the cursor on the [num] line
  * \ef func num -> starts a editor for a function and puts the cursor
on the [num] line
  * \sf func -> shows a full CREATE FUNCTION statement for the function
  * \sf+ func -> the same, but with line numbers
  * \sf[+] func num -> the same, but only from line num onward

It only touches psql, so no performance or backend stability worries.

In my humble opinion, only the \sf[+] is interesting, because it gives
you a copy/pasteable version of the function definition without opening
up an editor, and I can find that useful (OTOH: you can set PSQL_EDITOR
to cat and get the same effect with \ef... ok, just joking). Line
numbers are an extra touch, personally it does not thrill me too much,
but I've nothing against it.

The number variants of \e and \ef work by simply executing $EDITOR +num
file. I tried with some editors that came to my mind, and not all of
them support it (most do, though):

  * emacs and emacsclient work
  * vi works
  * nano works
  * pico works
  * mcedit works
  * kwrite does not work
  * kedit does not work

not sure what other people (or for instance Windows people) use. Apart
from no universal support from editors, it does not save that many
keystrokes - at most a couple. In the end you can usually easily jump to
the line you want once you are inside your dream editor.

My recommendation would be to only integrate the \sf[+] part of the
patch, which will have the additional benefit of making it much smaller
and cleaner (will avoid the grotty splitting of the number from the
function name, for instance). But I'm just another user out there, maybe
others will find uses for the other cases.

I would personally not add the leading and trailing newlines to \sf
output, but that's a question of taste.

Docs could use some small grammar fixes, but other than that they're fine.

== Code ==

In \sf code there just a strncmp, so this works:
\sfblablabla funcname

The error for an empty \sf is not great, it should probably look more like
\sf: missing required argument
following the examples of \pset, \copy or \prompt.

Why is lnptr always being passed as a pointer? Looks like a unnecessary
complication and one more variable to care about. Can't we just pass lineno?

== End ==

Cheers,
Jan