| Message ID | 20240910041013.68948-4-ericsunshine@charter.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a13ff419636c65321aee71ed000574a4a607be56 |
| Headers | show |
| Series | [v2,1/3] chainlint: don't be fooled by "?!...?!" in test body | expand |
On Tue, Sep 10, 2024 at 12:10:13AM -0400, Eric Sunshine wrote: > From: Eric Sunshine <sunshine@sunshineco.com> > > When chainlint detects a problem in a test definition, it highlights the > offending code with a "?!...?!" annotation. The rather curious "?!" > decoration was chosen to draw the reader's attention to the problem area > and to act as a good "needle" when using the terminal's search feature > to "jump" to the next problem. > > 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. > Together with the preceding change which gave all problem annotations a > uniform "LINT:" prefix, the noisy "?!" decoration has become superfluous > as a search "needle" so omit it when output is colored. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > t/chainlint.pl | 3 ++- > t/test-lib.sh | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/t/chainlint.pl b/t/chainlint.pl > index ad26499478..f0598e3934 100755 > --- a/t/chainlint.pl > +++ b/t/chainlint.pl > @@ -651,6 +651,7 @@ sub check_test { > $self->{nerrs} += @$problems; > return unless $emit_all || @$problems; > my $c = main::fd_colors(1); > + my ($erropen, $errclose) = -t 1 ? ("$c->{rev}$c->{red}", $c->{reset}) : ('?!', '?!'); > my $start = 0; > my $checked = ''; > for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) { I was first wondering why we didn't have to change our tests. But this seems to use either coloring or the `?!` decorations based on whether or not we output to a terminal. And as our tests output to a non-terminal they indeed see the old format, and as such they don't have to change. One thing I don't like about this is that we now have different output depending on whether or not you happen to pipe output to e.g. less(1), which I do quite frequently. So I'd propose to just drop the markers unconditionally. Patrick
On Tue, Sep 10, 2024 at 3:48 AM Patrick Steinhardt <ps@pks.im> wrote: > On Tue, Sep 10, 2024 at 12:10:13AM -0400, Eric Sunshine wrote: > > [...] > > 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. > > Together with the preceding change which gave all problem annotations a > > uniform "LINT:" prefix, the noisy "?!" decoration has become superfluous > > as a search "needle" so omit it when output is colored. > > > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > > --- > > + my ($erropen, $errclose) = -t 1 ? ("$c->{rev}$c->{red}", $c->{reset}) : ('?!', '?!'); > > I was first wondering why we didn't have to change our tests. But this > seems to use either coloring or the `?!` decorations based on whether or > not we output to a terminal. And as our tests output to a non-terminal > they indeed see the old format, and as such they don't have to change. Correct. > One thing I don't like about this is that we now have different output > depending on whether or not you happen to pipe output to e.g. less(1), > which I do quite frequently. So I'd propose to just drop the markers > unconditionally. My knee-jerk reaction is that the "?!" decoration is still handy for drawing the eye when scanning non-colored output visually (not using a search feature), so I'm hesitant to drop it. However, on reflection, I'm not sure I feel very strongly about it. What do others think?
Eric Sunshine <sunshine@sunshineco.com> writes: >> One thing I don't like about this is that we now have different output >> depending on whether or not you happen to pipe output to e.g. less(1), >> which I do quite frequently. So I'd propose to just drop the markers >> unconditionally. > > My knee-jerk reaction is that the "?!" decoration is still handy for > drawing the eye when scanning non-colored output visually (not using a > search feature), so I'm hesitant to drop it. However, on reflection, > I'm not sure I feel very strongly about it. What do others think? Unlike ERR, LINT is distinct enough, even when mixed with snippets taken from the test scripts that are full of words that hints errors, checking, etc., so I'd expect that new readers who have never seen the "?!" eye-magnets would not find the output too hard to read. For those of us whose eyes are so used to, we might miss them for a while, but I do not see much upside in keeping it. Thanks.
On Tue, Sep 10, 2024 at 11:42 AM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Tue, Sep 10, 2024 at 3:48 AM Patrick Steinhardt <ps@pks.im> wrote: > >> One thing I don't like about this is that we now have different output > >> depending on whether or not you happen to pipe output to e.g. less(1), > >> which I do quite frequently. So I'd propose to just drop the markers > >> unconditionally. > > > > My knee-jerk reaction is that the "?!" decoration is still handy for > > drawing the eye when scanning non-colored output visually (not using a > > search feature), so I'm hesitant to drop it. However, on reflection, > > I'm not sure I feel very strongly about it. What do others think? > > Unlike ERR, LINT is distinct enough, even when mixed with snippets > taken from the test scripts that are full of words that hints > errors, checking, etc., so I'd expect that new readers who have > never seen the "?!" eye-magnets would not find the output too hard > to read. For those of us whose eyes are so used to, we might miss > them for a while, but I do not see much upside in keeping it. Okay, thanks for weighing in. I'll reroll and make [3/3] drop the "?!" decoration unconditionally.
diff --git a/t/chainlint.pl b/t/chainlint.pl index ad26499478..f0598e3934 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -651,6 +651,7 @@ sub check_test { $self->{nerrs} += @$problems; return unless $emit_all || @$problems; my $c = main::fd_colors(1); + my ($erropen, $errclose) = -t 1 ? ("$c->{rev}$c->{red}", $c->{reset}) : ('?!', '?!'); my $start = 0; my $checked = ''; for (sort {$a->[1]->[2] <=> $b->[1]->[2]} @$problems) { @@ -659,7 +660,7 @@ sub check_test { my $err = format_problem($label); $checked .= substr($body, $start, $pos - $start); $checked .= ' ' unless $checked =~ /\s$/; - $checked .= "$c->{rev}$c->{red}?!LINT: $err?!$c->{reset}"; + $checked .= "${erropen}LINT: $err$errclose"; $checked .= ' ' unless $pos >= length($body) || substr($body, $pos, 1) =~ /^\s/; $start = $pos; diff --git a/t/test-lib.sh b/t/test-lib.sh index 54247604cb..278d1215f1 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 'LINT' annotations above)" fi # Last-minute variable setup