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

On 21/07/10 14:43, Pavel Stehule wrote:
> Hello
>
> I am sending a actualised patch.

Hi, thanks!

> I understand to your criticism about line numbering. I have to
> agree. With line numbering the patch is longer. I have a one
> significant reason for it.

> **** CREATE OR REPLACE FUNCTION public.foo() **** RETURNS integer
> **** LANGUAGE plpgsql **** AS $function$ 1 begin 2 return
> 10/0; 3 end; **** $function$
>
> postgres=# select foo(); ERROR: division by zero CONTEXT: SQL
> statement "SELECT 10/0" PL/pgSQL function "foo" line 2 at RETURN
> postgres=#

OK, that convinced me, and also others seem to like the feature. So I'll
just make code remarks this time.

In the \e handling code, if the file name was given and there is no line
number, an uninitialised variable will be passed to do_edit. I see that
you want to avoid passing a magic number to do_edit, but I think you
should just treat anything <0 as that magic value, initialise lineno
with -1 and simplify all these nested ifs.

Typo in a comment:
when wirst row of function -> when first row of function

I think the #define for DEFAULT_BODY_SEPARATOR should either be at the
beginning of the file (and then also used in \ef handling) or just
hardcoded in both places.

Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?

Some lines have >80 characters.

If you agree that a -1 parameter do do_edit will mean "no lineno", then
I think you can change get_lineno_for_navigation to not take a
backslashResult argument and signal errors by returning -1.

In get_lineno_for_navigation you will have to protect the large comment
block with /*------ otherwise pgindent will reflow it.

PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.

Uff, that's all from me, sorry for bringing up all these small issues, I
hope this will go in soon!

I'm going to change it to waiting on author again, mainly because of the
uninitialised variable in \d handling, the rest are just stylistic nitpicks.

Cheers,
Jan