On Thu, Apr 25, 2019 at 10:29 PM Tom Lane <[email protected]> wrote: > Alexander Korotkov <[email protected]> writes: > > I'm going to commit these adjustments if no objections. > > Sorry for not getting to this sooner. Looking quickly at the v2 patch, > it seems like you didn't entirely take to heart the idea of preferring > a useful primary error message over a boilerplate primary with errdetail. > In particular, in places like > > - errmsg(ERRMSG_SINGLETON_JSON_ITEM_REQUIRED), > - errdetail("expression should return a singleton boolean"))); > + errmsg("singleton SQL/JSON item required"), > + errdetail("Singleton boolean result is expected."))); > > why bother with errdetail at all? It's not conveying any useful increment > of information. In this example I think > > errmsg("expression should return a singleton boolean") > > is sufficient and well-phrased. Likewise, a bit further down, > > + errmsg("SQL/JSON member not found"), > + errdetail("JSON object does not contain key > \"%s\".", > > there is nothing being said here that wouldn't fit perfectly well into > one errmsg.
Makes sense. Attached revision of patch gets rid of errdetail() where
it seems to be appropriate.
> > My question regarding jsonpath_yyerror() vs. bison errors is still
> > relevant. Should we bother making bison-based errdetail() a complete
> > sentence starting from uppercase character? If not, should we make
> > other yyerror() calls look the same? Or should we rather move bison
> > error from errdetail() to errmsg()?
>
> The latter I think. The core lexer just presents the yyerror message
> as primary:
>
> scanner_yyerror(const char *message, core_yyscan_t yyscanner)
> {
> ...
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s at end of input", _(message)),
> lexer_errposition()));
>
> and in a quick look at what jsonpath is doing, I'm not really seeing
> the point of it being different. You could do something like
>
> /* translator: %s is typically the translation of "syntax error" */
> errmsg("%s in jsonpath input", _(message))
>
> to preserve the information that this is about jsonpath, but beyond
> that I don't see that splitting off an errdetail is helping much.
I've moved error message into errmsg().
> Or, perhaps, provide an errdetail giving the full jsonpath input string?
> That might or might not be redundant with other context information,
> so I'm not sure how useful it'd be.
I'm also not sure about this. Didn't do this for now.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
jsonpath-errors-improve-3.patch
Description: Binary data
