We have already tried the one policy at a time strategy, when we set the level of the Objects::ProhibitIndirectSyntax policy to be level-5. That worked well

Let's try something else now.

Set severity = 4, exclude some policies

We can look at the list of policy violations and and instead of fixing them, we can first just exclude them. The format we defined earlier now comes very handy. Each line includes the name of the violation. We can copy those names and put them in the .perlcriticrc file with a dash in-front of them. That means they are excluded from being checked.

After going over the report, excluding some violations, running perlcritic again and repeating this several times, I reach this .perlcriticrc file:

# please alpha sort config items as you add them

severity = 4
theme = core
verbose = %f: [%p] %m at line %l, column %c.\n

[Objects::ProhibitIndirectSyntax]
severity = 5

[-InputOutput::RequireBriefOpen]
[-Modules::ProhibitMultiplePackages]
[-Modules::RequireEndWithOne]
[-Modules::RequireExplicitPackage]
[-Subroutines::ProhibitBuiltinHomonyms]
[-Subroutines::RequireArgUnpacking]
[-Subroutines::RequireFinalReturn]
[-TestingAndDebugging::RequireUseWarnings]
[-ValuesAndExpressions::ProhibitCommaSeparatedStatements]
[-ValuesAndExpressions::ProhibitConstantPragma]
[-Variables::RequireLocalizedPunctuationVars]

After that I could run perlcritic . and it did not report any violation.

commit.

Now we can check for these policies one-by-one, fix the some of the modules and then go on. Once I fix all the code for a specific policy, I can remove that policy from the exclude list.

InputOutput::RequireBriefOpen

InputOutput::RequireBriefOpen might not be an earth shattering policy, but it can make it easier to read the code. In a nutshell, it stipulates that we should call close soon after we called open. Sounds reasonable.

This commit fixed it in two modules, but the issue remained in some of the test scripts in the FileCmp functions. I did not want to fix those as I think I am going to replace those functions by a module that will have better error reporting if and when the compared files are not the same.

So I kept the policy in the exclude list.

Modules::ProhibitMultiplePackages

Modules::ProhibitMultiplePackages requires that we won't have more than one package in every module.

Running perlcritic --single-policy Modules::ProhibitMultiplePackages . revealed that we have this issue in t/mapper.t and in lib/Pod/Tree/HTML.pm. Actually we had the same issue in several other modules but we have already factored out several packages to their own files. So we only had to fix one module.

Learning from the earlier mistakes, this time I remembered that I have to add lib/Pod/Tree/HTML/LinkMap.pm, the new filename to MANIFEST. I've also included our $VERSION = '1.23'; in the new file to satisfy the version number rules. I also added a 1; at the end of the new file. Just because I am quite used to that.

I did not want to fix the test script at this time as that's not even a module by itself. We'll see what to do there later.

commit

Modules::RequireExplicitPackage

At first I was not sure what does the Modules::RequireExplicitPackage policy really want in the modules. I know it complains about script that don't have package keyword and that's fine, but as far as I remembered all of the pm files in the distribution had a package statement in them. Then after reading the documentation I understood. The package statement needs to be the first statement in the file. Only comments and POD can precede it.

This makes sense. I'd go further and I'd even like to enforce a certain order of the use statement, but I am not sure if there is such policy. Maybe I could write one.

In any case, in this commit the package statements were moved to the beginning of the pm files.

TestingAndDebugging::RequireUseWarnings

TestingAndDebugging::RequireUseWarnings requires that every file have use warnings; at the beginning. (After the package statement if it is in the file.)

We have mostly implemented this already, but apparently there was one more file that did not have use warnings; in it.

This commit fixed it. I could also remove the [-TestingAndDebugging::RequireUseWarnings] from the .perlcriticrc file.

Subroutines::RequireArgUnpacking

The idea behind Subroutines::RequireArgUnpacking is that one should not access @_ or $_[..] in a middle of a function. The content of @_ need to be copied ("unpacked") into local lexical variables. That makes sense as this puts all the argument handling at the top of the function. I'd go even further, at least in code I maintain, and restrict it to the use of @_ (no shift), but let's go one step at a time.

In most of the modules, the issue could be fixed just by moving the assignment of @_ to the first row of the subroutine. In one module though I even had to introduce a new variable, so the code won't access $_[0] in the middle of the subroutine.

Release of version 1.24

With this we have arrived to the release of version 1.24 of Pod::Tree. We had to update the Changes file and the version number in every module, and then we could release the module to CPAN.

commit