Re: WIP patch for consolidating misplaced-aggregate checks

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Aug 9, 2012 at 11:56 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If we do go with the %s-for-a-SQL-keyword approach, it would then become
>> tempting to force-fit all of the cases into that style.

> I don't really like this, though. I don't think an error cursor is a
> good substitute for a clear statement of the categorical rule; or to
> put that another way, I think that forcing all of those messages into
> this model is going to be awkward.

Fair enough. I was not sold on doing that either. I would still like
to know if it's okay to use one string with %s for the cases where
there's not a good reason for the "context" to be more than just a
SQL keyword. That would save a few lines of code and also reduce
the number of strings for translators to deal with; so if it's not
horrid from a translation-quality standpoint, it seems worth doing.

One other thing I forgot to mention is that there was already some use
of error messages with a "constructName" string in almost the same
places, and in fact in the draft patch transformWhereClause and
transformLimitClause have *both* exprKind and constructName arguments,
which is kind of ugly and redundant. The reason I left the
constructName arguments in place is that currently they're fed to
coerce_to_boolean and/or coerce_to_specific_type, and it did not seem
very practical to convert those two functions to use exprKind instead.
The problem is that they are also used from within expression trees,
with constructNames like "AND", "OR", "NOT". We don't want the exprKind
to change when descending into AND sub-clauses for instance, so I didn't
see a good way to unify those usages with the overall clause type.
Anybody have an idea for improvement there?

regards, tom lane