Re: review: psql: edit function, show function commands patch - Mailing list pgsql-hackers
From | Pavel Stehule |
---|---|
Subject | Re: review: psql: edit function, show function commands patch |
Date | |
Msg-id | AANLkTimcnQzE69t-ZdAMYXBT3OWrzD=enDK3GHakBubr@mail.gmail.com Whole thread Raw |
In response to | Re: review: psql: edit function, show function commands patch (Pavel Stehule <pavel.stehule@gmail.com>) |
Responses |
Re: review: psql: edit function, show function commands
patch
|
List | pgsql-hackers |
Hello 2010/8/3 Pavel Stehule <pavel.stehule@gmail.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@gmail.com>: >> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.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 >> >
Attachment
pgsql-hackers by date: