Re: [PATCH 2/2] chainlint: reduce annotation noise-factor

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 29, 2024, 10:03 UTC
Message-ID: <ZtBHecRkFQkSAF6C@tanuki>
In-Reply-To: <20240829091625.41297-3-ericsunshine@charter.net>

On Thu, Aug 29, 2024 at 05:16:25AM -0400, Eric Sunshine wrote:

Show 32 quoted lines

> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> When chainlint detects a problem in a test definition, it highlights the
> offending code with an "?!ERR ...?!" annotation. The rather curious "?!"
> delimiter was chosen to draw the reader's attention to the problem area.
> 
> Later, chainlint learned to color its output when sent to a terminal.
> Problem annotations are colored with a red background which stands out
> well from surrounding text, thus easily draws the reader's attention. As
> such, the additional "?!" decoration became superfluous (when output is
> colored), however the decoration was retained since it serves as a good
> needle when using the terminal's search feature to "jump" to the next
> problem.
> 
> Nevertheless, the "?!" decoration is noisy and ugly and makes it
> unnecessarily difficult for the reader to pluck the problem description
> from the annotation. For instance, it is easier to see at a glance what
> the problem is in:
> 
>     ERR missing '&&'
> 
> than in the noisier:
> 
>     ?!ERR missing '&&'?!
> 
> Therefore drop the "!?" decoration when output is colored (but retain it
> otherwise).
> 
> Note that the preceding change gave all problem annotations a uniform
> "ERR" prefix which serves as a reasonably suitable replacement needle
> when searching in a terminal, so loss of "?!" in the output should not
> be overly problematic.

Okay, now the "ERR" prefix becomes a bit more important because we drop the other punctuation. I'm still not much of a fan of it, though. Makes me wonder whether we want to take a clue from how compilers nowadays format this, e.g. by using "pointers".

So this:

    2 (
    3 	foo |
    4 	bar |
    5 	baz &&
    6 
    7 	fish |
    8 	cow ?!AMP?!
    9 
    10 	sunder
    11 )

Would become this:

    t/chainlint/pipe.actual:8: error: expected ampersands (&&)
    7 	fish |
    8 	cow 
            ^
    9

While this would be neat, I guess it would also be way more work than the current series you have posted. And whether that work is ultimately really worth it may be another question. Probably not.

So overall, I'm fine with the direction that your patch series takes.

Patrick