review: psql: edit function, show function commands patch
in [PgSql]
|
From: Pavel Stehule on 3 Aug 2010 05:24 Hello 2010/8/3 Pavel Stehule <pavel.stehule(a)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(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 >> >
From: Pavel Stehule on 3 Aug 2010 07:20 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.stehule(a)gmail.com>: > Hello > > 2010/8/3 Pavel Stehule <pavel.stehule(a)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(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 >>> >> >
From: Robert Haas on 4 Aug 2010 10:29 On Wed, Aug 4, 2010 at 10:10 AM, David Fetter <david(a)fetter.org> wrote: > On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote: >> 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: >> >> 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. > > That concern is a show-stopper. See downthread; I believe we have a resolution to this issue. -- 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: Tom Lane on 4 Aug 2010 10:35 David Fetter <david(a)fetter.org> writes: > On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote: >> A side question is whether this should be an environment variable or >> a psql variable. > I'd say "yes." As with $EDITOR/PSQL_EDITOR, there should be something > that looks for an overriding psql variable, drops through to look for > an environment variable, and then to a sane default, for some > reasonable value of "sane." Perhaps this default could depend on OS > (Windows vs. Everything Else) to start with. Well, the thing about $EDITOR is that it's a very-widely-understood convention. This one won't be, so the argument for making it an environment variable seems pretty thin. It'd be saner to set it in your ~/.psqlrc file than to add another few nanoseconds to every process launch you ever do. 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: Greg Stark on 4 Aug 2010 20:50
On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Well, the thing about $EDITOR is that it's a very-widely-understood > convention. �This one won't be, so the argument for making it an > environment variable seems pretty thin. Fwiw the +linenumber convention has been part of $EDITOR since basically as long as vi has existed. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |