Re: review: psql: edit function, show function commands patch
От | Pavel Stehule |
---|---|
Тема | Re: review: psql: edit function, show function commands patch |
Дата | |
Msg-id | AANLkTimtr_y9BXVkmkoHwq4RPgx3Qkjx+2vqEU8Deeqm@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: review: psql: edit function, show function commands patch (Jan Urbański <wulczer@wulczer.org>) |
Ответы |
Re: review: psql: edit function, show function commands patch
(Jan Urbański <wulczer@wulczer.org>)
|
Список | pgsql-hackers |
Hello 2010/7/23 Jan Urbański <wulczer@wulczer.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
Вложения
В списке pgsql-hackers по дате отправления: