From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Jan Urbański <wulczer(at)wulczer(dot)org> |
Cc: | Postgres - Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: review: psql: edit function, show function commands patch |
Date: | 2010-07-23 18:55:39 |
Message-ID: | AANLkTimtr_y9BXVkmkoHwq4RPgx3Qkjx+2vqEU8Deeqm@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello
2010/7/23 Jan Urbański <wulczer(at)wulczer(dot)org>:
> 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.
>
fixed uninitialised variable. Second is little bit different - there
is three states, not only two - lineno is undefined, lineno is wrong
and lineno is correct. I would not to ignore a wrong lineno.
> Typo in a comment:
> when wirst row of function -> when first row of function
fixed
>
> 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.
this means some like only local constant - see PARAMS_ARRAY_SIZE in
same file. It is used only inside a first_row_is_empty function. It's
not used outside this function.
>
> Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?
>
no it is useless - fixed
> Some lines have >80 characters.
there are lot of longer lines - but I can't to modify other lines only
for formating. My code has max line about 90 characters (when it
doesn't respect more common form for some parts).
>
> 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.
there are a too much magic constants, so I prefer a cleaner form with
backslashResult. The code isn't longer and it reacts better on wrong
entered value - negative value is nonsense for this purpose.
>
> In get_lineno_for_navigation you will have to protect the large comment
> block with /*------ otherwise pgindent will reflow it.
done
>
> PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.
>
I changed PSQL_NAVIGATION_COMMAND to PSQL_EDITOR_NAVIGATION_OPTION and
append a few lines - as I can - some one who can speak English has to
correct it.
>
> Uff, that's all from me, sorry for bringing up all these small issues, I
> hope this will go in soon!
>
It is your job :)
> 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
>
attached updated patch
Thank you very much
Pavel Stehule
Attachment | Content-Type | Size |
---|---|---|
editfce.diff | text/x-patch | 19.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2010-07-23 18:55:56 | Re: permission inconsistency with functions |
Previous Message | Joshua D. Drake | 2010-07-23 18:48:50 | permission inconsistency with functions |