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

Hello

2010/8/3 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> attached updated patch
>
> * don't use a default option for navigation in editor - user have to
> set this option explicitly
> * name for this psql variable is EDITOR_LINENUMBER_SWITCH -
> * updated comments, doc and some issues described by Robert
>
> Regards
>
> Pavel Stehule

I found a small bug - the last patch better handle parsing lineno
after function descriptor

Regards

Pavel

>
> 2010/8/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> b) more robust algorithm for header rows identification
>>>
>>> Have not gotten to this one yet.
>>
>> I notIce that on WIN32 the default editor is notepad.exe and the
>> default editor navigation option is "/".  Does "notepad.exe /lineno
>> filename" actually work on Windows?  A quick Google search suggests
>> that the answer is "no", which seems somewhat unfortunate: it means
>> we'd be shipping "broken on Win32 out of the box".
>>
>> http://www.robvanderwoude.com/commandlineswitches.php#Notepad
>>
>> This is actually my biggest concern about this patch - that it may be
>> just too much of a hassle to actually make it work for people.  I just
>> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
>> that TextEdit doesn't understand +.  I'm afraid that we're going to
>> end up with a situation where it only works for people using emacs or
>> vi, and everyone else ends up with a broken install (and, possibly, no
>> clear idea how to fix it).  Perhaps it would be better to accept \sf
>> and reject \sf+ and \ef <func> <lineno>.
>>
>> Assuming we get past that threshold issue, I'm also wondering whether
>> the "navigation option" terminology is best; e.g. set
>> PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
>> terrible, but it doesn't seem great, either.  Anyone have a better
>> idea?
>>
>> The docs are a little rough; they could benefit from some editing by a
>> fluent English speaker.  If anyone has time to work on this, it would
>> be much appreciated.
>>
>> Instead of "line number is unacceptable", I think you should write
>> "invalid line number".
>>
>> "dollar" should not be spelled "dolar".  "function" should not be
>> spelled "finction".
>>
>> This change looks suspiciously like magic.  If it's actually safe, it
>> needs a comment explaining why:
>>
>> -       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
>> +       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
>>
>> Attempting to compile with this patch applied emits a warning on my
>> machine; whether or not the warning is valid, you need to make it go
>> away:
>>
>> command.c: In function ‘HandleSlashCmds’:
>> command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
>> command.c:1055: note: ‘bsep’ was declared here
>>
>> Why does the \sf output have a trailing blank line?  This seems weird,
>> especially because \ef puts no such trailing blank line in the editor.
>>
>> Instead of:
>>
>> +       /* skip useles whitespaces */
>> +       while (c >= func)
>> +               if (isblank(*c))
>> +                       c--;
>> +               else
>> +                       break;
>>
>> ...wouldn't it be just as good to write:
>>
>> while (c >= func && isblank(*c))
>>    c--;
>>
>> (and similarly elsewhere)
>>
>> In extract_separator, if you invert the sense of the first if-test,
>> then you can avoid having to indent the entire function contents.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise Postgres Company
>>
>