Re: [PATCH v2] CodingGuidelines: a handful of error message guidelines

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 28, 2024, 07:57 UTC
Message-ID: <CAPig+cSXGreymhni_J8TZ56rp+VZB_Z7ekaRtwFdxE3a2QWS5w@mail.gmail.com>
In-Reply-To: <xmqqa5dkqjmr.fsf_-_@gitster.g>

On Wed, Nov 27, 2024 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> It is more efficient to have something in the coding guidelines
> document to point at, when we want to review and comment on a new
> message in the codebase to make sure it "fits" in the set of
> existing messages.
>
> Let's write down established best practice we are aware of.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -689,16 +689,29 @@ Program Output
>  Error Messages
>
> - - Say what the error is first ("cannot open %s", not "%s: cannot open")
> + - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
> +
> + - Enclose the subject of an error inside a pair of single quotes,
> +   e.g. `die(_("unable to open '%s'"), path)`.
> +
> + - Unless there is a compelling reason not to, error messages from the
> +   Porcelain command should be marked for `_("translation")`.

Here you capitalize "Porcelain" but below, "plumbing" is all lowercase.

> + - Error messages from the plumbing commands are sometimes meant for
> +   machine consumption and should not be marked for `_("translation")`
> +   to keep them 'grep'-able.

Using the same example, `_("translation")`, for both the "should be" and "should not be" cases may very well confuse readers. (It certainly confused me.) Perhaps mirroring the example of an item earlier in the list would be clearer:

    - Unless there is a compelling reason not to, error messages from
      porcelain commands should be marked for translation, e.g.
      `die(_("bad revision"))`

    - Error messages from plumbing commands are sometimes meant for
      machine consumption, thus should not be marked for translation,
      e.g. `die("bad revision")`

> + - BUG("message") are for communicating the specific error to
> +   developers, and not to be translated.

Okay, although could be slightly more explicit:

    - BUG("message") is for communicating a specific failure to
      developers, not end-users, thus should not be translated.