Re: [PATCH 2/2] chainlint: reduce annotation noise-factor
From: Junio C Hamano <hidden>
Date: 2024-08-29 15:55:19
Eric Sunshine [off-list ref] writes:
From: Eric Sunshine <redacted>
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).Wait. That does not qualify "Therefore". We talked about a "good needle" and then complained how ugly the string that was happened to be chosen as good needle is. That is not enough to explain why it is justified to "lose" the needle. The only thing you justified is to move away from the ugly pattern, as a typical "terminal's search feature" does not give us an easy way to "jump to the next text painted yellow".
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.
Drop this separate paragraph, promote its contents up from "Note"
status and as a proper part of the previous sentence in its rewrite,
something like:
Since the errors are all uniformly prefixed with "ERR", which
can be used as the "good needle" instead, lose the "!?"
decoration when output is colored.
to replace "Therefore" and everything that follow.
quoted hunk
@@ -663,7 +666,7 @@ sub check_test { $checked =~ s/^\d+ \n//; $checked =~ s/(\s) \?!/$1?!/mg; $checked =~ s/\?! (\s)/?!$1/mg; - $checked =~ s/(\?![^?]+\?!)/$c->{rev}$c->{red}$1$c->{reset}/mg; + $checked =~ s/\?!([^?]+)\?!/$erropen$1$errclose/mg;
Hmph. With $erropen and $errclose, I was hoping that we can shed the reliance on the "?!" mark even internally. This is especially true that in the early part of this sub, the problem description was very much structured piece of data, not something the consuming code need to pick out of an already formatted text like this, risking to get confused by the payload (i.e. the text that came from the problematic test script inside "substr($body, $start, $pos-$start)" may contain anything, including "?!", right?).
quoted hunk
$checked =~ s/^\d+/$c->{dim}$&$c->{reset}/mg; $checked .= "\n" unless $checked =~ /\n$/; push(@{$self->{output}}, "$c->{blue}# chainlint: $title$c->{reset}\n$checked");@@ -805,9 +808,9 @@ sub check_script { my $c = fd_colors(1); my $s = join('', @{$parser->{output}}); $emit->("$c->{bold}$c->{blue}# chainlint: $path$c->{reset}\n" . $s); - $nerrs += () = $s =~ /\?![^?]+\?!/g; } $ntests += $parser->{ntests}; + $nerrs += $parser->{nerrs}; } return [$id, $nscripts, $ntests, $nerrs]; }diff --git a/t/test-lib.sh b/t/test-lib.sh index 54247604cb..b652cb98cd 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh@@ -1606,7 +1606,7 @@ if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 && test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0 then "$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" || - BUG "lint error (see '?!...!? annotations above)" + BUG "lint error (see 'ERR' annotations above)" fi # Last-minute variable setup
Overall the two patches looked great. Thanks.