Horton

Thread

Commits

  1. Prohibit map and grep in void context

  1. perlcritic: prohibit map and grep in void conext

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-07-27T16:06:19Z

    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

  2. Re: perlcritic: prohibit map and grep in void conext

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-27T19:09:10Z

    > 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/

  3. Re: perlcritic: prohibit map and grep in void conext

    Michael Paquier <michael@paquier.xyz> — 2021-07-28T04:23:12Z

    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

  4. Re: perlcritic: prohibit map and grep in void conext

    Andrew Dunstan <andrew@dunslane.net> — 2021-07-28T11:10:29Z

    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

  5. Re: perlcritic: prohibit map and grep in void conext

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-28T11:26:23Z

    > 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/

  6. Re: perlcritic: prohibit map and grep in void conext

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-08-07T23:15:47Z

    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

  7. Re: perlcritic: prohibit map and grep in void conext

    Michael Paquier <michael@paquier.xyz> — 2021-08-27T06:10:23Z

    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

  8. Re: perlcritic: prohibit map and grep in void conext

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-27T07:15:43Z

    > 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/

  9. Re: perlcritic: prohibit map and grep in void conext

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-08-27T10:32:30Z

    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

  10. Re: perlcritic: prohibit map and grep in void conext

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-30T18:27:09Z

    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

  11. Re: perlcritic: prohibit map and grep in void conext

    Michael Paquier <michael@paquier.xyz> — 2021-08-31T01:22:52Z

    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

  12. Re: perlcritic: prohibit map and grep in void conext

    Julien Rouhaud <rjuju123@gmail.com> — 2021-08-31T04:19:12Z

    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

  13. Re: perlcritic: prohibit map and grep in void conext

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-31T09:30:19Z

    > 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/

  14. Re: perlcritic: prohibit map and grep in void conext

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-08-31T09:55:59Z

    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