Thread: perlcritic: prohibit map and grep in void conext
perlcritic: prohibit map and grep in void conext
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Hi hackers, In the patches for improving the MSVC build process, I noticed a use of `map` in void context. This is considered bad form, and has a perlcritic policy forbidding it: https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::ProhibitVoidMap. Attached is a patch that increases severity of that and the corresponding `grep` policy to 5 to enable it in our perlcritic policy, and fixes the one use that had already snuck in. - ilmari From c82b08ce047ab47188753806984f1dc5be365556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 27 Jul 2021 16:51:29 +0100 Subject: [PATCH] perlcritic: prohibit map and grep in void context --- contrib/intarray/bench/create_test.pl | 2 +- src/tools/perlcheck/perlcriticrc | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/contrib/intarray/bench/create_test.pl b/contrib/intarray/bench/create_test.pl index 993a4572f4..ae8d72bab0 100755 --- a/contrib/intarray/bench/create_test.pl +++ b/contrib/intarray/bench/create_test.pl @@ -51,7 +51,7 @@ else { print $msg "$i\t{" . join(',', @sect) . "}\n"; - map { print $map "$i\t$_\n" } @sect; + print $map "$i\t$_\n" foreach @sect; } } close $map; diff --git a/src/tools/perlcheck/perlcriticrc b/src/tools/perlcheck/perlcriticrc index e230111b23..9267fb43b2 100644 --- a/src/tools/perlcheck/perlcriticrc +++ b/src/tools/perlcheck/perlcriticrc @@ -22,3 +22,10 @@ verbose = %f: %m at line %l, column %c. %e. ([%p] Severity: %s)\n # insist on use of the warnings pragma [TestingAndDebugging::RequireUseWarnings] severity = 5 + +# forbid grep and map in void context +[BuiltinFunctions::ProhibitVoidGrep] +severity = 5 + +[BuiltinFunctions::ProhibitVoidMap] +severity = 5 -- 2.30.2
> On 27 Jul 2021, at 18:06, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Attached is a patch that increases severity of that and the > corresponding `grep` policy to 5 to enable it in our perlcritic policy, > and fixes the one use that had already snuck in. +1, the use of foreach also improves readability a fair bit IMO. -- Daniel Gustafsson https://vmware.com/
On 7/27/21 12:06 PM, Dagfinn Ilmari Mannsåker wrote: > Hi hackers, > > In the patches for improving the MSVC build process, I noticed a use of > `map` in void context. This is considered bad form, and has a > perlcritic policy forbidding it: > https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::ProhibitVoidMap. > > Attached is a patch that increases severity of that and the > corresponding `grep` policy to 5 to enable it in our perlcritic policy, > and fixes the one use that had already snuck in. > Personally I'm OK with it, but previous attempts to enforce perlcritic policies have met with a less than warm reception, and we had to back off. Maybe this one will fare better. I keep the buildfarm code perlcritic compliant down to severity 3 with a handful of exceptions. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 28 Jul 2021, at 13:10, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 7/27/21 12:06 PM, Dagfinn Ilmari Mannsåker wrote: >> Hi hackers, >> >> In the patches for improving the MSVC build process, I noticed a use of >> `map` in void context. This is considered bad form, and has a >> perlcritic policy forbidding it: >> https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::ProhibitVoidMap. >> >> Attached is a patch that increases severity of that and the >> corresponding `grep` policy to 5 to enable it in our perlcritic policy, >> and fixes the one use that had already snuck in. > > Personally I'm OK with it, but previous attempts to enforce perlcritic > policies have met with a less than warm reception, and we had to back > off. Maybe this one will fare better. I'm fine with increasing this policy, but I don't have strong feelings. If we feel the perlcritic policy change is too much, I would still prefer to go ahead with the map rewrite part of the patch though. -- Daniel Gustafsson https://vmware.com/
Re: perlcritic: prohibit map and grep in void conext
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > Hi hackers, > > In the patches for improving the MSVC build process, I noticed a use of > `map` in void context. This is considered bad form, and has a > perlcritic policy forbidding it: > https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::ProhibitVoidMap. > > Attached is a patch that increases severity of that and the > corresponding `grep` policy to 5 to enable it in our perlcritic policy, > and fixes the one use that had already snuck in. Added to the 2021-09 commitfest: https://commitfest.postgresql.org/34/3278/ - ilmari
> On 27 Aug 2021, at 08:10, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 28, 2021 at 01:26:23PM +0200, Daniel Gustafsson wrote: >> I'm fine with increasing this policy, but I don't have strong feelings. If we >> feel the perlcritic policy change is too much, I would still prefer to go ahead >> with the map rewrite part of the patch though. > > I have no issue either about the rewrite part of the patch, so I'd > tend to just do this part and move on. Daniel, would you like to > apply that? Sure, I can take care of that. -- Daniel Gustafsson https://vmware.com/
Michael Paquier <michael@paquier.xyz> writes: > On Wed, Jul 28, 2021 at 01:26:23PM +0200, Daniel Gustafsson wrote: >> I'm fine with increasing this policy, but I don't have strong feelings. If we >> feel the perlcritic policy change is too much, I would still prefer to go ahead >> with the map rewrite part of the patch though. > > I have no issue either about the rewrite part of the patch, so I'd > tend to just do this part and move on. Daniel, would you like to > apply that? Why the resistance to the perlcritic part? That one case is the only violation in the tree today, and it's a pattern we don't want to let back in (I will surely object every time I see it when reviewing patches), so why not automate it? - ilmari
On 8/27/21 6:32 AM, Dagfinn Ilmari Mannsåker wrote: > Michael Paquier <michael@paquier.xyz> writes: > >> On Wed, Jul 28, 2021 at 01:26:23PM +0200, Daniel Gustafsson wrote: >>> I'm fine with increasing this policy, but I don't have strong feelings. If we >>> feel the perlcritic policy change is too much, I would still prefer to go ahead >>> with the map rewrite part of the patch though. >> I have no issue either about the rewrite part of the patch, so I'd >> tend to just do this part and move on. Daniel, would you like to >> apply that? > Why the resistance to the perlcritic part? That one case is the only > violation in the tree today, and it's a pattern we don't want to let > back in (I will surely object every time I see it when reviewing > patches), so why not automate it? > There doesn't seem to have been much pushback, so let's try it and see. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Tue, Aug 31, 2021 at 9:23 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Aug 30, 2021 at 02:27:09PM -0400, Andrew Dunstan wrote: > > There doesn't seem to have been much pushback, so let's try it and see. > > Okay, fine by me. +1
> On 31 Aug 2021, at 06:19, Julien Rouhaud <rjuju123@gmail.com> wrote: > > On Tue, Aug 31, 2021 at 9:23 AM Michael Paquier <michael@paquier.xyz> wrote: >> >> On Mon, Aug 30, 2021 at 02:27:09PM -0400, Andrew Dunstan wrote: >>> There doesn't seem to have been much pushback, so let's try it and see. >> >> Okay, fine by me. > > +1 Since there is concensus in the thread with no -1’s, I’ve pushed this to master now. -- Daniel Gustafsson https://vmware.com/
On Tue, 31 Aug 2021, at 10:30, Daniel Gustafsson wrote: > > On 31 Aug 2021, at 06:19, Julien Rouhaud <rjuju123@gmail.com> wrote: > > > > On Tue, Aug 31, 2021 at 9:23 AM Michael Paquier <michael@paquier.xyz> wrote: > >> > >> On Mon, Aug 30, 2021 at 02:27:09PM -0400, Andrew Dunstan wrote: > >>> There doesn't seem to have been much pushback, so let's try it and see. > >> > >> Okay, fine by me. > > > > +1 > > Since there is concensus in the thread with no -1’s, I’ve pushed this to master > now. Thanks! - ilmari