Re: [HACKERS] plpgsql - additional extra checks

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: nospam-abuse(at)bloodgate(dot)com, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Marko Tiikkaja <marko(at)joh(dot)to>, Jim Nasby <jim(dot)nasby(at)bluetreble(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] plpgsql - additional extra checks
Date: 2018-07-12 10:32:13
Message-ID: CAFj8pRA+yRtMVjYZA7Lefns6hmYMDBUe38PNuNCFyWPzuQ_G-A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi

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.
>

should be fixed now.

Regards

Pavel

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

Attachment Content-Type Size
plpgsql-extra-check-180712-1.patch text/x-patch 18.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2018-07-12 10:42:31 Re: ON CONFLICT DO NOTHING on pg_dump
Previous Message Heikki Linnakangas 2018-07-12 10:15:03 Re: Temporary WAL segments files not cleaned up after an instance crash