Re: review: psql: edit function, show function commands patch

Hello

2010/7/16 Jan Urbański <wulczer(at)wulczer(dot)org>:
> 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.
>

I disagree. When I working on servers of my customers there are some
default configuration - default editor is usually vi or vim. I cannot
change my preferred editor there and \ef n - can help me very much (I
know only one command for vi - :q :)). I am looking on KDE. There is
usual parameter --line.

I propose following solution - to add a system variable
PSQL_EDITOR_GOTOLINE_COMMAND that will contains a command for editors
without general +n navigation.

so you can set a PSQL_EDITOR_GOTOLINE_COMMAND='--line %d'

and when this string will be used, when will not be empty without default.

> 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.

Maybe better is using a title - so source code will use a format like
standard result set.

I'll look on it.

>
> 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

will be fiexed

>
> 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?
>

because I would not to use a magic constant. when lnptr is NULL, then
lineno is undefined.

Thank you very much

Pavel Stehule

> == End ==
>
> Cheers,
> Jan
>