Helping to clean CPAN: July 2016
… 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: Module::Path
Module::Path: return the full path of a locally installed module.
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 26th of Oct 2015 (https://github.com/neilb/Module-Path)
- latest release 16th of Mar 2015 (https://metacpan.org/release/Module-Path)
- 6 open issues in GitHub
- 1 open pull requests
- 0 open issues on RT (https://rt.cpan.org/Public/Dist/Display.html?Name=Module-Path)
- 11 reverse dependencies via metacpan
- 2 test failures on cpantesters: http://www.cpantesters.org/distro/M/Module-Path.html
- one using cperl
- perl couldn’t be found on Windows
- CPANTS report
- cpants didn’t know anything about the module; not clear if there’s a problem with CPANTS at present, or if the module is somehow excluded from it
Initial inspection of the source code
After forking the repo and cloning a local copy, let’s 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).
- no README.md (fixed in PR#24)
- Travis config could be updated to more Perl versions (fixed in PR#18)
- uses
Dist::Zilla
- there is no
META.yml
orMETA.json
in the repository; looks like they’re automatically generated, since they’re in the dist cpanm --installdeps .
works okdzil test
showed thatdzil authordeps --missing | cpanm
was necessary on my boxdzil test
runs ok
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:
94.1% statement coverage; 79.6% total coverage
Which is pretty good.
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.
Run podchecker
and check for errors and/or warnings.
$ find ./lib -name '*.pm' | xargs podchecker |& grep ERROR # 2 errors
*** ERROR: unresolved internal link 'perlfunc"/"require' at line 100 in file ./lib/Module/Path.pm
*** ERROR: unresolved internal link 'perlfunc"/"require' at line 143 in file ./lib/Module/Path.pm
$ find ./lib -name '*.pm' | xargs podchecker |& grep WARN # 6 warnings
*** WARNING: No argument for =item at line 92 in file ./lib/Module/Path.pm
*** WARNING: No argument for =item at line 97 in file ./lib/Module/Path.pm
*** WARNING: No argument for =item at line 103 in file ./lib/Module/Path.pm
*** WARNING: No argument for =item at line 109 in file ./lib/Module/Path.pm
*** WARNING: No argument for =item at line 114 in file ./lib/Module/Path.pm
*** WARNING: No items in =over (at line 165) / =back list at line 169 in file ./lib/Module/Path.pm
Fixed in PR#20.
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)
There was only one file to change. Fixed in PR #19.
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
- code before strictures warnings found
$ perlcritic t
"return" statement with explicit "undef" at line 59, column 5. See page 199 of PBP. (Severity: 5)
Removing the explicit undef fixes the issue and looks like it will maintain the required behaviour (the tests also still pass).
Fixed in PR#22.
Find 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://'
$ git grep 'ftp://'
$ git grep 'git://'
All links look good.
Check copyright notices
This probably also sits in the nit-picking category for some people, however, copyright dates (theoretically) need to be kept up to date. The appropriate copyright year is usually the year of the last release. However, if a release looks imminent, then the current year is likely to be the right candidate. Some distributions put the author’s email address on the same line as the copyright date, hence this needs to be checked as well.
Files missing current year in copyright
Here we do a case-insensitive grep over the source for the word “copyright”,
the line of which we check for the existence of a year (i.e. 4 digits), look
for the appropriate year and then clean up the grep results to get something
we can pass directly to vim
.
$ COPYRIGHT_YEAR=2016
$ git grep -i copyright
$ vim $(git grep -i copyright | grep -P '\d{4}' | grep -v $COPYRIGHT_YEAR | cut -d':' -f1 | uniq)
There were 3 files to investigate. Fixed in PR#21.
Files missing an email address in copyright
$ vim $(git grep -i copyright | grep -P '\d{4}' | grep -v '@' | cut -d':' -f1 | uniq)
There is one file which appears in this search which is dist.ini
and
doesn’t need the email address on the same line.
Spell check files with 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.
POD spell checks ok.
Kwalitee tests
Although CPANTS is the main kwalitee
reference, one can also run the kwalitee tests locally. One can use the
t/kwalitee.t
test script from
http://peateasea.de/cpan-pull-request-challenge/ for this purpose.
However, the script only uses Test::Kwalitee
which doesn’t cover as many
metrics as CPANTS.
Test::Kwalitee::Extra uses
set of metrics closer to that used on CPANTS, so replace the kwalitee_ok
call with simply use Test::Kwalitee::Extra
. More information about the
many options to Test::Kwalitee::Extra
can be found on the MetaCPAN page.
Run the kwalitee tests in an ExtUtils::MakeMaker
or Build::Module
distribution like so:
$ RELEASE_TESTING=1 prove t/99kwalitee.t
or, if the distribution uses Dist::Zilla
, run
$ dzil test --author --release
This test complains that there isn’t a README
, a MANIFEST
, a build file
or a META.yml
. I believe all of these things are automatically generated.
After having run
$ dzil build
and having changed into the created directory, and running the tests there, then the kwalitee tests pass.
GitHub issues
Tried to address issue #17 by adding the dirs option to the module_path
program in #PR27. The pull request is an attempt at getting close to the
right implementation, it isn’t intended as a correct and good way to solve
the problem, however will hopefully fuel the inspiration to get a good
solution implemented.
RT issues
There are no issues in RT.
Overview of the pull requests made
- extend list of Perls tested in Travis (https://github.com/neilb/Module-Path/pull/18)
- merged!
- remove trailing whitespace (https://github.com/neilb/Module-Path/pull/19)
- merged!
- fix podchecker issues (https://github.com/neilb/Module-Path/pull/20)
- update the copyright year (https://github.com/neilb/Module-Path/pull/21)
- remove explicit return of undef (https://github.com/neilb/Module-Path/pull/22)
- ignore dzil auto-generated files (https://github.com/neilb/Module-Path/pull/23)
- add a README.md in main project (https://github.com/neilb/Module-Path/pull/24)
- fix minor pod problems (https://github.com/neilb/Module-Path/pull/25)
- add dirs option to
module_path
(https://github.com/neilb/Module-Path/pull/27)
Conclusion
Even though some of the pull requests were reviewed and merged very quickly, the remaining ones are still waiting for review.
Many thanks to NEILB for merging the PRs and to DOLMEN for feedback on some of the contributions.