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
- some
- ran
dzil listdeps --missing
- get a warning that
NoTabsTest
should be replaced withTest::NoTabs
- get a warning that
- 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
- warnings are thrown about
[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 theDist::Zilla
plugins. Unfortunately, setting the--automated
flag to thedzil 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. Andgit config --list
doesn’t mention theuser.name
oruser.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
andPod::Coverage::TrustPod
need to be installed as a prereq. Where to put this information indist.ini
? - come to think of it, why are the pod-coverage tests running on Travis
when the
--release
option isn’t passed todzil 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 wordauthor
on Travis, but with the wordrelease
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 fromDist::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 thatGit::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 viadzil 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 betweenDist::Zilla
on my local box and on the Vagrant and Travis machines. There had been a change inDist::Zilla
behaviour which meant thatdzil 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
withTest::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!