Thread: Error detail/hint style fixup

When fixing review comments for error message style on another patch, I noticed
that there were a few error details/hints/contexts that weren’t following the
style guide in the documentation.  This might be intentional from when they
were added, or we intentionally avoid changing after the fact to not impact
translations too much, or we just don’t care (didn’t find any past mention in
the archives though), but my OCD cannot unsee it now so I figured I'd send it
and see.

Attached patch ensures that (i) details and hints have leading capitalization,
have double spaces after punctuation and ends with period; (ii) context should
not be capitalized and should not end with period; (iii) test .out files match
the changes.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
> Attached patch ensures that (i) details and hints have leading capitalization,
> have double spaces after punctuation and ends with period; (ii) context should
> not be capitalized and should not end with period; (iii) test .out files match
> the changes.

+1 for cleaning this up, but I wonder if you've gone far enough in
adjusting the style of errcontext() messages.  The style guide says
"Context strings should normally not be complete sentences", with
the expectation that we're typically going to concatenate several
of them into what amounts to a stack trace.  And while the guide
doesn't say in so many words "describe the context in which the
error occurred", that's surely what you're supposed to do.
So I'm thinking that, eg,

-         errcontext("Error occurred on dblink connection named \"%s\": %s.",
+         errcontext("error occurred on dblink connection named \"%s\": %s",
                     dblink_context_conname, dblink_context_msg)));

is not getting the job done; at least the "error occurred" part is simply
redundant given that this is a context string.  Looking at the actual uses
of this, eg

 NOTICE:  relation "foobar" does not exist
-CONTEXT:  Error occurred on dblink connection named "unnamed": could not open cursor.
+CONTEXT:  error occurred on dblink connection named "unnamed": could not open cursor

I'm thinking what we should actually be printing is more like

CONTEXT:  while opening cursor on dblink connection named "unnamed"

That'd require fixing the callers of this subroutine too, but maybe
it's worth doing.

            regards, tom lane

> On 19 Mar 2018, at 17:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Attached patch ensures that (i) details and hints have leading capitalization,
>> have double spaces after punctuation and ends with period; (ii) context should
>> not be capitalized and should not end with period; (iii) test .out files match
>> the changes.
>
> +1 for cleaning this up, but I wonder if you've gone far enough in
> adjusting the style of errcontext() messages.

Thanks for looking/reviewing.

> I'm thinking what we should actually be printing is more like
>
> CONTEXT:  while opening cursor on dblink connection named “unnamed"

I agree that the contexts in dblink were pretty unhelpful with redundant
language.  I took a stab at this, the attached patch extends dblink_res_error()
to improve the context.  Including the cursorname in the context seemed to be
in line with the errmsg’s in dblink.

Looking around at other errcontext’s I can’t see any other cases that warrant
attention.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
> [ errdetail_hint_style_v2.patch ]

I started going through this in more detail, and I see that a significant
chunk of the changes are to put two spaces not one between sentences in
errdetail/errhint messages.  This is per our style guideline:

    Detail and hint messages: Use complete sentences, and end each with
    a period.  Capitalize the first word of sentences.  Put two spaces after
    the period if another sentence follows (for English text; might be
    inappropriate in other languages).

but I wonder if maybe the right answer is to drop the last sentence of
that style guideline.  It seems a bit at odds with the general principle
enunciated under Formatting: "Don't put any specific assumptions about
formatting into the message texts".  There are those who consider it
obsolete style, too, eg

http://www.thedailymash.co.uk/news/society/last-human-to-use-two-spaces-after-a-full-stop-dies-20180312145785

Personally I do type two spaces, which is a habit I learned while using
TeX (where it made a difference), but that was a long time ago.

A quick grep through one of the backend .po files suggests that about
two-thirds of our messages where the issue arises have one space rather
than two (I counted about 35 instances of one space, 15 of two).  If
we did want to standardize this fully, we'd have to hit more places
than you did here.

In short, I'm not sure it's worth thrashing a lot of our translatable
strings to enforce a debatable style detail.

Thoughts?

            regards, tom lane

> On 22 Mar 2018, at 17:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> In short, I'm not sure it's worth thrashing a lot of our translatable
> strings to enforce a debatable style detail.

I thought I had voiced that exact concern in my previous email, but I totally
missed that.

Being a two-space guy I would like the style to remain that, but I also agree
that the churn is way too expensive and that it’s considered quite obsolete and
old by many.  Cutting that change from the patch makes the remainder more
palatable.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
> Being a two-space guy I would like the style to remain that, but I also agree
> that the churn is way too expensive and that it’s considered quite obsolete and
> old by many.  Cutting that change from the patch makes the remainder more
> palatable.

This stuff seems reasonably non-controversial, so pushed.

I couldn't resist the temptation to mess about with dblink_res_error
a bit more, too.

BTW, really the point of what I'd mentioned before was to avoid having
dblink_res_error constructing a message out of fragments, which it's
still doing.  I'd thought perhaps we would shove the responsibility for
mentioning the connection name out to the callers to get rid of that.
But handling the possibility of an unnamed connection seems like it'd
complicate the callers considerably.  And as long as we don't actually
have translation support in that module, it'd just be make-work, so
I didn't do it.

            regards, tom lane

> On 22 Mar 2018, at 22:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> This stuff seems reasonably non-controversial, so pushed.

Thanks!

> BTW, really the point of what I'd mentioned before was to avoid having
> dblink_res_error constructing a message out of fragments, which it's
> still doing.  I'd thought perhaps we would shove the responsibility for
> mentioning the connection name out to the callers to get rid of that.
> But handling the possibility of an unnamed connection seems like it'd
> complicate the callers considerably.  And as long as we don't actually
> have translation support in that module, it'd just be make-work, so
> I didn't do it.

Right, that would be a lof of complexity for no real gain until dblink is
translated.

cheers ./daniel