From: | Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: jsonpath |
Date: | 2019-05-07 04:14:40 |
Message-ID: | CAPpHfdt4EbbV=wupVSLS2M_sbVv3uCs40PrrvYkfP-gauPRMzA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Apr 30, 2019 at 1:20 AM Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> On Mon, Apr 29, 2019 at 6:11 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > > [ jsonpath-errors-improve-3.patch ]
> >
> > This is getting better, but IMO it's still a bit too willing to use
> > a boilerplate primary error message plus errdetail. I do not think
> > that is project style nor something to be encouraged.
> >
> > In particular, you've got a whole lot of cases like this:
> >
> > + errmsg("JSON object is expected"),
> > + errdetail("Jsonpath member accessor can only be applied to an object."))));
> >
> > I think you should just drop the errmsg and use the errdetail as primary
> > (with no-initcap and no-trailing-period, of course). I don't see more
> > than two or three cases in this whole patch where I'd use an errdetail
> > at all; in almost all of them, the proposed errdetail looks perfectly
> > suitable to be a primary message.
>
> Ok, now it seems that I understood. errdetail is removed from vast
> majority of cases.
>
> > One other generic gripe is that a lot of these messages use the term
> > "singleton", which seems a bit too jargon-y to me. As far as I can
> > see in a quick look at the backend .po files, we have not up to now
> > used that term in *any* user-facing error message. Nor does it appear
> > anywhere in our user-facing documentation, except for one place that
> > was itself inserted by the jsonpath patch:
> >
> > Arrays of size 1 are interchangeable with a singleton.
> >
> > I don't think that's either obvious to a non-mathematician, or
> > even technically correct; maybe it'd be better as
> >
> > An array of size 1 is considered equal to its sole element.
> >
> > Likewise, I think it'd be better to avoid "singleton" in the error
> > messages. In some places you could perhaps use "single" instead.
> > In some you just don't need it at all, eg in
> > Jsonpath array subscript is not a singleton numeric value.
> > you could just drop the word "singleton" and it'd be perfectly
> > correct, since a numeric is necessarily a single value.
> >
> > Also, we do often use the term "scalar" to mean a non-composite
> > value; maybe that would work for this context, in places where
> > you do really need that meaning.
>
> Makes sense for me. "Singleton" word comes from the standard. But
> assuming we almost don't use it in the documentation (and especially
> don't define it), it's better to get rid of this word altogether.
> Removed from error messages. Separate patch adjusting docs as you
> proposed is also attached.
>
> > Sorry to be making you work so hard on this, but I think good
> > error messages are an important part of having a quality feature.
> > I do see a lot of improvements already compared to where we started.
>
> It's nothing to be sorry about. I need to learn this in order to make
> my further commits better. Thank you for your explanations.
Attached patchset contains revised commit messages. I'm going to
commit this on no objections.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment | Content-Type | Size |
---|---|---|
0001-jsonpath-errors-improve-5.patch | application/octet-stream | 41.6 KB |
0002-remove-singleton-word-from-docs-5.patch | application/octet-stream | 1.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2019-05-07 04:28:36 | Re: accounting for memory used for BufFile during hash joins |
Previous Message | Michael Paquier | 2019-05-07 04:06:05 | Re: Identity columns should own only one sequence |