Bernd Helmle <[EMAIL PROTECTED]> writes: > please find attached a patch which implements psql command aliases. They > work the same way as on bash, zsh and others, for example:
I'm still not convinced that we want this sort of feature at all. I quote from the current bash manual page: ALIASES ... The rules concerning the definition and use of aliases are somewhat confusing. ... For almost every purpose, aliases are superseded by shell functions. The bash authors seem to wish they'd never adopted the feature, which makes me wonder whether we really want to copy it slavishly. Having said that, though, here are some code-review points: * The patch intends that the expansion of an alias could be either a backslash command or SQL command text, but I don't think you've thought through the interactions of these cases. In particular, the patch doesn't seem to behave very sanely if the backslash command comes when there is already text on the current line or previous lines of the query buffer: it just wipes that text out, which is surely not the desirable thing. * Use of a VariableSpace for the stack of aliases-being-expanded seems exceedingly klugy: variable spaces are intended for persistent storage not transient state, so this fails to satisfy the principle of least astonishment for readers of the code. Also, you're failing to manipulate it as a stack: you shouldn't wipe a stack entry upon matching it, only when control returns from having expanded it. Another problem is that it doesn't get reset at all if the alias expansion is just SQL text and not another backslash command, meaning successive uses of such an alias won't work. I think you'd be better off if the expansion were done as a separate recursive routine with the set of active aliases just passed as a list threaded through local variables of the routine invocations. * I think you should lose the separate PSQL_CMD_ALIAS status code and just use PSQL_CMD_NEWEDIT. The one place where the codes act different looks like a mistake to me anyway. * Don't feed non-constant strings through gettext(): + if (cmd) + { + puts(_(cmd)); + success = true; + } The best you can hope for is that it doesn't do anything. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches