5 years on, we’re still benefiting from Perl::Critic — but we’ve changed approach
5 years ago, I shared how we had recently started using Perl::Critic, specifically with Test::Perl::Critic::Progressive, to gradually improve a large codebase.
Today, we can say that quality and standardisation has improved, but there are still more improvements we can make.
We’ve also changed the way that we use Perl::Critic. One of the reasons for this was speed. A separate initiative sped up our application’s main test suite from over an hour to under 10 minutes. That meant that running perl critic over all of the files was adding a significant amount of time to the build pipeline.
Because we’ve reduced the number of high priority failures to a very low number, we felt that we no longer need to use Test::Perl::Critic::Progressive. This tool allows you to forgive existing violations, while avoiding adding new ones. The previous post explains why we used it and some custom scripts we hacked up to make its error reports easier to read.
Our new approach involves running perl critic only on modified files, but requiring all violations in those files to be resolved.
Only criticising modified files
The main motivation for switching to only criticising modified files was to speed up the build process.
Our builds run on a git checkout, so it’s easy-ish to see which files have been modified. We use the same approach to speed up some other checks that are strictly per-file, such as compilation checks. The logic for detecting which files have changed is encapsulated in a module (provided below, in case it’s useful).
A secondary result of only criticising modified files is that we can leave some of the old, minor failures in unimportant modules (where unimportant means: doesn’t change) and focus on failures of new policies.
This approach also means that if a module that hasn’t changed for a long time is modified, we need to address any existing violations in that module. We are not afraid to make use of ## no critic where we think the violation is unimportant, or the module is so old that nobody knows how to test the fix!
Perl critic modules we’re rolling out
We now have all of our code tidied according to a perltidyrc, and each change is tested by https://metacpan.org/pod/Perl::Critic::Policy::CodeLayout::RequireTidyCode
We’ve also dealt with a number of pretty serious bug-causing violations such as https://metacpan.org/pod/Perl::Critic::Policy::Community::ConditionalDeclarations (this “accidentally” makes something that behaves a bit like a state variable, which is particularly bad in a web worker context!)
So now we install and/or increase the priority of policies when we happen to come across issues. For example we’re currently working on https://metacpan.org/pod/Perl::Critic::Policy::Community::DollarAB after a bug I recently caused due to the fact that $a and $b are exempt from use strict's checking that variables are declared!
Conclusions
- Perl Critic remains a useful tool
- Test::Perl::Critic::Progressive required some hacky scripts to provide sufficient diagnostics for us
- After making some progress with Test::Perl::Critic::Progressive, we changed to criticising modified files
Appendix: Detecting modified files
The following script is how we detect modified files in a branch. It may not be the best way but it has been working fine for us.
Adzuna::Policy just brings in use strict and use warnings for us (in this case).
The number of commits limitation is because we use a shallow git clone in our Jenkins nodes to speed up build checkouts.
https://github.com/Ovid/git-critic is an attempt to do this better, however at the time that we needed to speed up our build we couldn’t quite get it to work. I did try to patch the issue but failed. Better here means it only criticises modified lines, rather than the whole file. However there are some advantages to improving files when they change too!
