Re: review: psql: edit function, show function commands patch - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: review: psql: edit function, show function commands patch |
Date | |
Msg-id | AANLkTikB_YxYfuueqB_BTE===wVos38836b81oRp32Xf@mail.gmail.com Whole thread Raw |
In response to | Re: review: psql: edit function, show function commands patch (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: review: psql: edit function, show function commands patch
Re: review: psql: edit function, show function commands patch Re: review: psql: edit function, show function commands patch |
List | pgsql-hackers |
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
pgsql-hackers by date: