New Perl::Critic rules

[dipietroR]

Describe the bug

Thinking of adding [BuiltinFunctions::RequireBlockMap], [BuiltinFunctions::RequireBlockGrep], and [InputOutput::RequireBriefOpen]

This would involve updating the code to match: https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::RequireBlockMap I'm not so clear on if we want to do this as it looks more like a readability thing than something performance based. Still having things in braces looks nice.

Another part of this would be: https://metacpan.org/pod/Perl::Critic::Policy::InputOutput::RequireBriefOpen Where we should close filehandles once we are done with them. For me, this one seems more valuable to do.

To Reproduce

Run perlcritic --severity 4

Expected behavior

Perlcritic would report file as ok.

Screenshots

No response

Additional context

For more context: https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::RequireBlockMap https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::RequireBlockGrep https://metacpan.org/pod/Perl::Critic::Policy::InputOutput::RequireBriefOpen

Type of device

Other

Browser version

No response

Number of products impacted

No response

Time per product

No response

[alexgarel]

For me the "RequireBriefOpen" might be the most interesting. But it may require to refactor lots of code part in a heavy way (eg. changing loops in file lines to isolate file iterations in an iterator function).

[stephanegigandet]

Those rules sound good to me, we have very few map and grep, so it should be easy to do.

For the open files, I'm curious to see how many of them will be flagged. It may force use to refactor and create functions for the processing, but that's a good thing.

[alexgarel]

"InputOutput" name space seems interesting.

We may also explore the "Variables" and "ValuesAndExpressions" rules. As they may have interesting rules. For example

• Perl::Critic::Policy::Variables::ProhibitReusedNames - Do not reuse a variable name in a lexical scope

[alexgarel]

I wanted to find a rule that would spot the auto vivification or lack of variable declaration (we have this in Tags.pm, the variables used by load_taxonomy but couldn't find it !

[Ban3]

I looked at RequireBriefOpen when we were doing the severity 4 PR, and IIRC there were few subroutines that would need significant refactoring to pass the default close within 9 lines. I like the policy, but it's not trivial like the grep/map one.

Beside readability, block grep/map scales better because you can just add statements to the block.

[Ban3]

These are the policies in the Perl-Critic distribution with the default severity of 4:

• BuiltinFunctions::RequireBlockGrep Write grep { /$pattern/ } @list instead of grep /$pattern/, @list.
• BuiltinFunctions::RequireBlockMap Write map { /$pattern/ } @list instead of map /$pattern/, @list.
• CodeLayout::RequireConsistentNewlines Use the same newline through the source.
• ControlStructures::ProhibitLabelsWithSpecialBlockNames Don’t use labels that are the same as the special block names.
• ControlStructures::ProhibitUnreachableCode Don’t write code after an unconditional die, exit, or next.
• ControlStructures::ProhibitYadaOperator Never use ... in production code.
• InputOutput::ProhibitExplicitStdin Use "<>" or "<ARGV>" or a prompting module instead of "<STDIN>".
• InputOutput::ProhibitOneArgSelect Never write select($fh).
• InputOutput::ProhibitReadlineInForLoop Write while( $line = <> ){...} instead of for(<>){...}.
• InputOutput::RequireBriefOpen Close filehandles as soon as possible after opening them.
• Modules::ProhibitAutomaticExportation Export symbols via @EXPORT_OK or %EXPORT_TAGS instead of @EXPORT.
• Modules::ProhibitMultiplePackages Put packages (especially subclasses) in separate files.
• Modules::RequireEndWithOne End each module with an explicitly 1; instead of some funky expression.
• Modules::RequireExplicitPackage Always make the package explicit.
• Objects::ProhibitIndirectSyntax Prohibit indirect object call syntax.
• Subroutines::ProhibitBuiltinHomonyms Don’t declare your own open function.
• Subroutines::RequireArgUnpacking Always unpack @_ first.
• Subroutines::RequireFinalReturn End every path through a subroutine with an explicit return statement.
• TestingAndDebugging::ProhibitNoWarnings Prohibit various flavors of no warnings.
• TestingAndDebugging::ProhibitProlongedStrictureOverride Don’t turn off strict for large blocks of code.
• TestingAndDebugging::RequireUseWarnings Always use warnings.
• ValuesAndExpressions::ProhibitCommaSeparatedStatements Don’t use the comma operator as a statement separator.
• ValuesAndExpressions::ProhibitConstantPragma Don’t use constant FOO => 15.
• ValuesAndExpressions::ProhibitMixedBooleanOperators Write !$foo && $bar || $baz instead of not $foo && $bar or $baz.
• Variables::ProhibitAugmentedAssignmentInDeclaration Do not write my $foo .= 'bar';.
• Variables::ProhibitMatchVars Avoid $`, $&, $' and their English equivalents.
• Variables::RequireLocalizedPunctuationVars Magic variables should be assigned as “local”.
• Variables::RequireNegativeIndices Negative array index should be used.

Out of these we have already disabled ProhibitMatchVars because they should not be an issue in newer Perls.

[dipietroR]

Sounds good. I'll start working on the grep, the map test, and ProhibitReusedNames. I agree looks like RequireBriefOpen would be hard to implement.

[dipietroR]

These are the policies in the Perl-Critic distribution with the default severity of 4:

Thank you for giving a list of them all!

[dipietroR]

Are there any other policies that pique anyone's interest?