Helping to clean CPAN: November 2015

… where I get out my virtual broom and sweep up cruft in my assigned distribution for this month’s edition of the CPAN Pull Request Challenge.

This month’s module: Plack::App::FakeApache1

Plack::App::FakeApache1 is a Perl distro to aid in mod_perl1->PSGI migration.

First impressions

In this section I try to get a feeling for the state of the module, how up to date it is, how often people are contributing to it, how many other distributions are depending on it, how many bugs/issues it currently has, what the CPANTS kwalitee is, etc.

• last commit in January 2014 (https://github.com/chiselwright/plack-app-fakeapache1)
• 0 pull requests outstanding, 2 closed, no PRs in 2015
• 2 open issues in GitHub; no RT queue
• latest release: 22nd of Jan, 2014 (https://metacpan.org/pod/Plack::App::FakeApache1)
• 1 reverse dependency via MetaCPAN
• Experimental kwalitee metrics failing on CPANTS (http://cpants.cpanauthors.org/release/CHISEL/Plack-App-FakeApache1-0.0.5)
  • META.yml needs provides
  • not all test-related modules are listed in build_requires

Initial inspection of the source code

After forking the repo and cloning a local copy, I have a look at the project to see what build system it uses, if the test suite works and the tests pass, if it could do with a Travis-CI config file (or if present, if it can be updated). The initial inspection often gives inspiration for the first batch of pull requests.

• Dist::Zilla project
• needs README.md to be helpful to people viewing GitHub project
• needs a proper Dist::Zilla abstract
• 12 failing tests; 246 passing, 8 n/a
• copyright info needs updating
• doesn’t provide a META.json
• ran dzil authordeps --missing | cpanm
  • some Dist::Zilla plugins were added
• ran dzil listdeps --missing
  • get a warning that NoTabsTest should be replaced with Test::NoTabs
• ran dzil listdeps --missing | cpanm
• dzil test runs and the tests pass (without the release tests)
  • warnings are thrown about PodWeaver not finding an abstract in some files
  • warnings from PkgVersion about no blank line for $VERSION after package statement

[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeApache1/Constants.pm
[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeApache1/Handler.pm
[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeApache1/Request.pm
[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeModPerl1.pm
[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeModPerl1/Dispatcher.pm
[PodWeaver] [@Default/Name] couldn't find abstract in lib/Plack/App/FakeModPerl1/Server.pm
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeApache1 statement in lib/Plack/App/FakeApache1.pm line 1
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeApache1::Constants statement in lib/Plack/App/FakeApache1/Constants.pm line 1
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeApache1::Handler statement in lib/Plack/App/FakeApache1/Handler.pm line 1
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeApache1::Request statement in lib/Plack/App/FakeApache1/Request.pm line 1
[PkgVersion] no blank line for $VERSION after package Moose::APR::Table statement in lib/Plack/App/FakeApache1/Request.pm line 148
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeModPerl1 statement in lib/Plack/App/FakeModPerl1.pm line 1
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeModPerl1::Dispatcher statement in lib/Plack/App/FakeModPerl1/Dispatcher.pm line 1
[PkgVersion] no blank line for $VERSION after package Plack::App::FakeModPerl1::Server statement in lib/Plack/App/FakeModPerl1/Server.pm line 1

• no .travis.yml -> added in PR#8
• dzil test --release
  • t/release-pod-coverage.t tests fail
• while adding a .travis.yml, found that the tests won’t pass without the git user.name and user.email settings being in the config. This is the fault of one of the Dist::Zilla plugins. Unfortunately, setting the --automated flag to the dzil test command doesn’t help. It would be nice if the distribution were able to take this into account. Ended up setting up a Vagrant VM to investigate the issue. Inside the VM the problem with the git setup doesn’t appear. And git config --list doesn’t mention the user.name or user.email settings. The error in Travis is:

*** Please tell me who you are.

Run

   git config --global user.email "you@example.com"
   git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

• Vagrant VM environment notes that Test::Pod::Coverage and Pod::Coverage::TrustPod need to be installed as a prereq. Where to put this information in dist.ini?
• come to think of it, why are the pod-coverage tests running on Travis when the --release option isn’t passed to dzil test??? It looks like the pod tests are being set as author tests on Travis, but as release tests when running on a local environment. It turned out this was because the test is prefixed with the word author on Travis, but with the word release on my local box.
• using dzil test --verbose on Travis unfortunately doesn’t uncover exactly where the git issue is coming from. It looks like it’s coming from Dist::Zilla itself, since it appears just after this message:

     [DZ] writing Plack-App-FakeApache1 in .build/....

• after checking the Dist::Zilla plugins via divide-and-conquer, it turns out that Git::CommitBuild is the plugin which causes the issue.
• an upgrade of Dist::Zilla on my local box made the author tests (for this distribution the POD tests) run per default via dzil test. This means that the pod-coverage issue is now repeatable on the local Vagrant VM, the local bare metal box and the Travis setup. At least now we’re consistent! The reason for the inconsistency was a version difference between Dist::Zilla on my local box and on the Vagrant and Travis machines. There had been a change in Dist::Zilla behaviour which meant that dzil test runs author tests by default, hence why on the more up to date systems the author tests were getting run.

POD checks

The utility podchecker searches through Perl source code for POD which might not conform to the POD standard, and thus not necessarily be parseable by all POD parsers. Fixing any issues found by podchecker has the positive effect of also removing any warnings noted in the project’s documentation displayed on MetaCPAN.

Running podchecker gives the following errors and warnings:

$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep ERROR # 3 errors
$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep WARN # 0 warnings

POD errors fixed in PR#10.

Nit-picking

Check for trailing whitespace

Some projects consider this a must, and will disallow commits to be submitted which contain trailing whitespace (the Linux kernel is an example project where trailing whitespace isn’t permitted). Other projects see whitespace cleanup as simply nit-picking. Either way one sees it personally, this could be a useful pull request to a project, so it’s worthwhile fixing and submitting; the worst that can happen is that the pull request is closed unmerged.

To look for files with trailing whitespace, run git grep ' $'. It can be helpful to load the files found directly into vim:

$ vim $(git grep ' $' | cut -d':' -f1 | sort | uniq)

Need to fix 7 files with trailing whitespace. Fixed in PR#9.

perlcritic

Perl::Critic will show up many potential issues for Perl code. By simply running perlcritic on the lib and t directories, one can get a further handle on the code’s quality.

$ perlcritic lib
$ perlcritic t

Found some strictures issues. Fixed one strictures issue. The two remaining issues aren’t really a problem (a warning about turning off a stricture and a warning about string eval).

Strictures issues fixed in PR#12.

Code Coverage

Looking at the code coverage can give an indication of code quality. If the project is well covered, this means most changes made in pull requests can be made with some confidence that any problems will be caught by the test suite. If the code coverage is low, then this is something that one could address as a pull request (or set of pull requests).

In EUMM and Build::Module projects, one simply needs to install Devel::Cover and run

$ cover -test

In Dist::Zilla projects, one needs to install the Dist::Zilla::App::Command::cover plugin, after which the code coverage can be checked via:

$ dzil cover

In this distribution, the coverage is:

29.9% total coverage

Which is quite low…

Stale URLs

Links to websites can go out of date, so it’s a good idea to see if they need updating or removing. A quick grep finds all the links. After which, we just need to see which links need fixing, if any.

$ git grep 'http://\|https://\|ftp://\|git://'

Everything seems to work and to point to the correct location.

Spell check POD

Good documentation can be a wonder to read. Not everyone’s docs are awesome, however we can keep the error rate to a minimum. A quick spell check will pick up most typos that don’t need to be there and fixing them can help improve the quality of a project.

In general, we want to find all files containing POD and run a spell checker (e.g. aspell) over all files, fixing typos we come across as we go. Not all projects require this much effort, however here’s a general-ish way to look for and check all POD in a project:

$ files_with_pod=$(find ./lib -name '*.pm' -o -name '*.pod' \
                 | xargs podchecker 2>&1 \
                 | grep 'pod syntax' | cut -d' ' -f1)
$ for filename in $files_with_pod
do
    pod2text $filename > $filename.txt;
    aspell -c $filename.txt;
done

Now look for .bak file and check differences between it and the output .txt file, the process looks roughly like this:

$ find ./ -name '*.bak'
$ diff -u lib/Path/To/Module.pm.pm.txt.bak lib/Path/To/Module.pm.txt

Then update the appropriate .pm and/or .pod files as necessary.

Fixed some typos found in the POD in PR#14.

GitHub issues

Tests fail with non-English locale (GitHub issue #5) => added a fix for the issue in PR #15.

Kwalitee tests

The module already uses Kwalitee tests as part of the --release option to dzil test, thus this doesn’t need to be followed much further.

Overview of the pull requests made

• replace NoTabsTest with Test::NoTabs
• add missing ABSTRACT statements to packages
• add Travis configuration
• purge trailing whitespace
• fix POD errors
• update the dist.ini
  • update the copyright date
  • don’t distribute vim backup files
  • ensure all test-related prereqs are installed (this should also ensure that the build prerequisites are specified as required by the CPANTS experimental metrics)
  • ensure that a META.json file is created
  • ensure that “provides” information is available in META.yml
• add missing strictures to test
• remove perl shebang line in test scripts
• fix typos in the POD
• fix GitHub issue #5 (Tests fail with non-English locale)
• implement part of a fix for GitHub issue #4 (error severity level issue)

Conclusion

Most PRs were merged on the first night (1st of Nov)! Many thanks to CHISEL for the quick reaction!