Skip navigation

Tag Archives: Perl::Critic

Don’t put Perl::Critic into source control pre-commit hooks.

Some people seem to like to put Perl::Critic tests into their VCS pre-commit hooks in order to prevent code with violations being checked in. I consider this to be a really bad idea. You should always be able to check your code in, if only for backup purposes. It’s a matter of preserving your code, even if it’s got some stupid PodSpelling violation. You’ve got a production emergency bug fix and you can’t check in because you’ve got a ProhibitEscapedMetacharacters problem? I certainly wouldn’t want that.

No Devel::Declare. Ever.

I think Devel::Declare is evil for the same reasons that Adam does.

Even if PPI supports it, Perl::Critic will not be supporting any syntax that’s not part of the language core. Not only because it would be necessary to figure out semantics, but because I oppose balkanization of the language.

To quote myself: “Please add a ‘class. keyword. Would that break MooseX::Declare? Yup. Tough cookies.“

Deprecation.

In the source repository, there are a number of changes to Perl::Critic that include deprecation of existing functions/methods. One Policy author has asked us to not do the deprecation because it will result in warnings being emitted whenever his Policy is being used.

There have been a lot of discussion recently in the Perl community about what deprecation means. For me, deprecation means that the interface or functionality will be removed and not sit around in a limbo state for years.

So how do I decide whether to deprecate something? If something is merely suboptimal, I’m not going to deprecate it; I’ll merely change the documentation to discourage its use. In Perl::Critic, there was the issue of Policy constructors. For a long time, there was no requirements for the new() method for a given Policy other than it returning a blessed hash. There are some Policys [sic] outside of the core distribution with new() methods that don’t call up to the new() method in Perl::Critic::Policy. There was a point when we really wanted to add a lot of functionality to the constructor, but we couldn’t because there were Policys that didn’t call it. We worked around this by doing what we wanted after the Policy object was constructed. If you look at the developer documentation for Perl::Critic, you’ll note that it discourages creation of a new() method.

On the other hand, if something is incorrectly designed or is holding up future changes, I’m going to change it or remove it. I’m not going to just work around the issue forever. I won’t change it without notifying anyone, however.

I made a mistake with the design of Perl::Critic::Utils::PPI::get_constant_name_element_from_declaring_statement(). I didn’t realize that you can actually create multiple values using the constant pragma; Tom Wyant corrected my misconception. The function returns a single value, so it can’t correctly handle single statements that declare multiple constants. Its interface is wrong. It will be removed in the future. A suggestion was made to immediately remove it, but we’ll go through a deprecation cycle first. We won’t change things without giving notice.

I think the Policy author’s question about the warnings comes from the practice of a number of projects of “deprecating” something, but never actually removing it. Sun is a particularly egregious example of this with Java functionality that has been deprecated for more than a decade but still hangs around, allowing people to write new code against an interface that is broken. If I deprecate something, it will be removed. The warnings that are emitted allow others to deal with the change before it happens. If, in the interim, this annoys some users, too bad. It would be worse if the code just stopped working.

If you have a look at my code page, among the epigrams that you find there is “If it’s broken, fix it.”. While there has long been the saying of not fixing something that isn’t broken, I think people don’t do the opposite enough. Don’t suffer breakage. Don’t put up with things that are wrong. Change things that need changing, even if it causes some short-term pain.

How I use Perl::Critic.

Under regular development conditions, I rarely use perlcritic. Most of my Perl::Critic usage is via Test::Perl::Critic for new code and Test::Perl::Critic::Progressive for existing code.

For code that I release to the CPAN, the Perl::Critic tests are in xt/author so that they don’t get run as part of normal tests that someone installing the modules runs. On someone else’s system, I don’t know what Perl::Critic version or add-on sets of policies they have, so those tests may fail there through no fault of my own.

For non-public code, the Perl::Critic tests are regular tests that get run with all the rest of the tests. I can do this because I control the specific version of Perl::Critic that I’m running and which add-on policies that I’ve got around.

Perl::Critic supports the concept of severity levels for policies and, by default, only applies the most severe ones (severity 5). I effectively ignore the severities by turning Perl::Critic’s strictness all the way to 1. Of course, there are policies that I don’t agree with or cannot use for practical reasons, so I disable those explicitly.

You may have noticed that above I used the word “tests” and not “test”. This is because I run Perl::Critic on my tests as well as my “real” code. The set of policies that I use on tests is different than I use on the rest of my code. Tests have fewer restrictions on them compared to regular code. So I have a test that applies to the contents of lib and bin using one perlcriticrc file and a seperate test that runs on the content of the t and xt directories with a different perlcriticrc. For tests, I disable the POD policies and Subroutines::ProtectPrivateSubs (tests are allowed to peek). Sometimes I also disable ValuesAndExpressions::ProhibitMagicNumbers, though I prefer not to.

(Perl::Critic actually uses three self-compliance tests: one for the regular code, one for the tests, and one that makes an additional pass over the Perl::Critic::Policy subclasses that places some additional requirements on them.)

The sets of add-on policies that I generally use are Perl::Critic::More, Perl::Critic::Bangs, Perl::Critic::Swift, and Perl::Critic::Moose. I’m thinking about starting to use Perl::Critic::Pulp, but I’d have to disable about half the policies in there.

There are a couple of cases in which I do use perlcritic.

First, if I’m surveying a codebase that is new to me, I will run perlcritic --statistics-only over it in order to get an idea of the state of affairs.

Second, presented with a set of code that has a large number of problems that need fixing, after running perlcritic --statistics-only on it, I’ll pick an individual policy that I want to fix violations of and run perlcritic -s policy name on the code. (The long form of -s is --single-policy.) I find it easier to fix one kind of problem across all of the files before moving on to another kind of problem than to fix all the problems in one file before moving on to another file.

Perl::Critic supports autodie.

In chromatic’s session this morning, there was a comment that Perl::Critic didn’t support autodie. It actually has supported it since New Year’s day of this year.

Let me say emphatically that one of the core Perl::Critic developers (i.e. me) loves autodie. If he could, he would marry it.

If you aren’t using autodie, please do so.

Please use autodie. PLEASE use autodie. PLEASE “use autodie;”!!!

(Or, even better “use autodie qw< :all >;”.)

What kind of personality does Perl::Critic have?

Probably pedantic, but we’ll see what it says.

Perl::Critic policies in the DarkPAN.

I’m curious about any people who’ve written custom Perl::Critic policies that aren’t on CPAN.

What sorts of things are these for?

Have you had a look at Perl::Critic 1.082? Have you read Perl::Critic::DEVELOPER? What do you think?

This post originally appeared at use.perl.org.

Writing configurable Perl::Critic Policies.

As of Perl::Critic 1.07, I would like to discourage the creation of constructors for Policies. Instead, I would encourage the use of P::C::Policy::initialize_if_enabled(). The reasoning is twofold.

One, this allows initialization to be deferred to the point where we know that the policy is going to be used. P::C::PolicyFactory always instantiates every Policy that it can find. It is up to the P::C::Config object to filter that set down. Primarily, this is an issue for Policies which dynamically load other modules.

Two, this method enables the Policy to decide for itself whether it should be enabled. This means that a Policy that depends upon a module that might not be present can remove itself from the set that violates() gets called on, thus speeding things up because it isn’t being called on every PPI::Element.

This originated from Chris Dolan’s work on the Perl Foundation grant to create the remaining Policies that can be implemented that enforce one of the ideas in PBP. Specifically, for Documentation::PodSpelling, but this change has been made to all the configurable core Policies. In particular, this helps CodeLayout::RequireTidyCode.

Differences from a constructor other than the obvious first parameter:

  1. The configuration is passed in as a hashref and not a hash an instance of Perl::Critic::PolicyConfig (corrected 2009/5/16).
  2. initialize_if_enabled() returns a boolean specifying whether the Policy should be enabled.

This is how the two above policies bow out.

This post originally appeared at use.perl.org.

Lesson of the day: Readonly, Exporter, and subroutine sigils

They don’t get along.

As of the recent 1.07 release, Perl::Critic, has started using Readonly to be more more self-compliant with Perl Best Practices. We had been avoiding use of constant for the reasons described in the book, but had not been willing to add the Readonly dependency until now.

The Perl::Critic coding standard has been to use sigils for subroutines in @EXPORT_OK, etc. and import lists, but Exporter treats them as optional. And, in fact, there’s code that strips them off (line 47 in v5.58). I haven’t figured out the commonality, but in a few environments, this fails spectacularly. Once we removed the subroutine sigils from everywhere, the problems have gone.

Explicitness: 0, Keyboard laziness: 1.

This post originally appeared at use.perl.org.