review: psql: edit function, show function commands patch
in [PgSql]
|
From: Tom Lane on 2 Aug 2010 22:49 Robert Haas <robertmhaas(a)gmail.com> writes: > 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). [ disclaimer: I've not looked at the proposed patch yet ] It seems like this ought to be fairly easily surmountable as long as the patch is designed for failure. The fallback position is just that the line number does nothing, ie \ef foo just opens the editor and doesn't try to position the cursor anywhere special; nobody can complain about that because it's no worse than before. What we need is to not try to force positioning if we don't recognize the editor. I'm tempted to suggest forgetting about any user-configurable parameter and just provide code that strcmp's the $EDITOR value to see if it recognizes the editor name, otherwise do nothing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 2 Aug 2010 20:54 On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 2 Aug 2010 23:34 On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas(a)gmail.com> writes: >> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: >>> I'm tempted >>> to suggest forgetting about any user-configurable parameter and just >>> provide code that strcmp's the $EDITOR value to see if it recognizes the >>> editor name, otherwise do nothing. > >> With all due respect, that sounds like an amazingly bad idea. �Surely, >> we'll be forever getting patches to add $MYFAVORITEEDITOR to the list, >> or complaints that it's not already included. > > Well, yeah, that's the idea. �I say that beats a constant stream of > complaints that $MYFAVORITEEDITOR no longer works at all --- which > is what your concern was, no? Well, it'd still work fine for \e foo. It'll just blow up for \e foo 3. My concern isn't really that things that which work now will break so much as that this new feature will fail to work for a large percentage of the people who try to use it, including virtually everyone who tries to use it on Win32. >> While this is >> superficially a Nice Thing to Have and I would certainly support it if >> +linenumber were relatively universal, it's really a pretty minor >> convenience when you come right down to it, and I am not at all >> convinced it is worth the hassle of trying to divine what piece of >> syntax will equip the user's choice of editor with the necessary >> amount of clue. > > The other approach we could take is that this whole thing is disabled by > default, and you have to set a psql variable EDITOR_LINENUMBER_SWITCH > to turn it on. �If you haven't read the documentation enough to find > out that variable exists, well, no harm no foul. That might be reasonable. Right now the default behavior is to do +line on Linux and /line on Windows. But maybe a more sensible default would be to fail with an error message that says "you have to set thus-and-so variable if you want to use this feature". That seems better than sometimes working and sometimes failing depending on what editor the user has configured. A side question is whether this should be an environment variable or a psql variable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 3 Aug 2010 00:52 2010/8/3 Robert Haas <robertmhaas(a)gmail.com>: > On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas(a)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 notapad supports nothing :( Microsoft doesn't use command line. I found this syntax in pspad. Next other editor is notepad++. It use a syntax -n. Wide used KDE editors use --line syntax > > 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>. > \sf+ is base - we really need a source output with line numbers. \ef foo func is just user very friendly function. I agree, so this topic have to be better documented > 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. > > -- I'' recheck these issue Pavel > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company > -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 3 Aug 2010 05:04
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 2010/8/3 Robert Haas <robertmhaas(a)gmail.com>: > On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas(a)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 > |