Re: [PATCH 01/18] t: add skeleton chainlint.pl

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 1, 2022, 12:27 UTC
Message-ID: <220901.86k06njmvq.gmgdl@evledraar.gmail.com>
In-Reply-To: <3423df94bd6035640828a2508968cf8e1f5b4dda.1661992197.git.gitgitgadget@gmail.com>

On Thu, Sep 01 2022, Eric Sunshine via GitGitGadget wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>
> [...]
> diff --git a/t/chainlint.pl b/t/chainlint.pl

I really like this overall direction...

> +use warnings;
> +use strict;

I think that in general we're way overdue for at least a :

	use v5.10.1;

Or even something more aggresive, I think we can definitely depend on a newer version for this bit of dev tooling.

That makes a lot of things in this series more pleasing to look at. E.g. you could use named $+{} variables for regexes.

> +package ScriptParser;

I really wish this could be changed to just put this in t/chainlint/ScriptParser.pm early on, we could set @INC appropriately and "use" these, which...

> +my $getnow = sub { return time(); };
> +my $interval = sub { return time() - shift; };

Would eliminate any scoping concerns about this sort of thing.

> +if (eval {require Time::HiRes; Time::HiRes->import(); 1;}) {
> +	$getnow = sub { return [Time::HiRes::gettimeofday()]; };
> +	$interval = sub { return Time::HiRes::tv_interval(shift); };
> +}

Is this "require" even needed, Time::HiRes is there since 5.7.* says "corelist -l Time::HIRes".

Show 9 quoted lines

> [...]
> +sub check_script {
> +	my ($id, $next_script, $emit) = @_;
> +	my ($nscripts, $ntests, $nerrs) = (0, 0, 0);
> +	while (my $path = $next_script->()) {
> +		$nscripts++;
> +		my $fh;
> +		unless (open($fh, "<", $path)) {
> +			$emit->("?!ERR?! $path: $!\n");

If we can depend on v5.10.1 this can surely become:

	use autodie qw(open close);

No?

> +			$nerrs += () = $s =~ /\?![^?]+\?!/g;

y'know if we add some whitespace there we can conform to https://metacpan.org/dist/perlsecret/view/lib/perlsecret.pod >:) (not serious...)