On Tue, Apr 30, 2019 at 1:20 AM Alexander Korotkov
<a.korot...@postgrespro.ru> wrote:
> On Mon, Apr 29, 2019 at 6:11 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <a.korot...@postgrespro.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
0001-jsonpath-errors-improve-5.patch
Description: Binary data
0002-remove-singleton-word-from-docs-5.patch
Description: Binary data
