perlcritic: prohibit map and grep in void conext
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
Attachments:
0001-perlcritic-prohibit-map-and-grep-in-void-context.patchtext/x-diffDownload+8-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 Tue, Jul 27, 2021 at 09:09:10PM +0200, Daniel Gustafsson wrote:
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.
Sounds interesting to avoid. pgperlcritic does not complain here
after this patch.
--
Michael
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/
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 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?
--
Michael
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 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.
--
Michael
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