Re: [HACKERS] plpgsql - additional extra checks

2018-07-09 21:44 GMT+02:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> > + ereport(errlevel,
> > (errcode(ERRCODE_TOO_MANY_
> ROWS),
> > errmsg("query returned
> more than one row"),
> > - errdetail ?
> errdetail_internal("parameters: %s", errdetail) : 0));
> > + errdetail ?
> errdetail_internal("parameters: %s", errdetail) : 0,
> > + use_errhint ?
> errhint("too_many_rows check of extra_%s is active.",
> > +
> too_many_rows_level == ERROR ? "errors" : "warnings") : 0));
>
> Please write this in a way that results in less translatable messages,
> and don't build setting names at runtime. Concretely I suggest this:
>
> errhint(too_many_rows_level == ERROR ?
> gettext_noop("%s check of extra_errors is active.") :
> gettext_noop("%s check of extra_warnings is active."),
> "too_many_rows");
>
> This way,
> 1. only two messages need translation, not one per type of warning/error
> 2. the translator doesn't need to scratch their head to figure out what
> the second %s is
> 3. they don't have to worry about introducing a typo in the string
> "too_many_rows" or the strings "extra_errors", "extra_warnings".
> (Laugh all you want. It's a real problem).
>
> If you can add a /* translator: */ comment to indicate what the first %s
> is, all the better. I'm just not sure *where* it needs to go. I'm not
> 100% sure the gettext_noop() is really needed either.
>
> > + ereport(strict_
> multiassignment_level,
> > +
> (errcode(ERRCODE_DATATYPE_MISMATCH),
> > +
> errmsg("Number of source and target fields in assignment do not match."),
>
> Please, no uppercase in errmsg(), and no ending period.
>

Thank you for notes. Tomorrow morning I'll spend few hours in train and
I'll send updated patch

Regards

Pavel

> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>