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

Hello

I hope so I found and fixed last issue - the longer functions was
showed directly - without a pager.

Regards

Pavel

2010/8/3 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 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
>>>
>>
>