Helping to clean CPAN: November 2016
… where I get out my virtual broom and sweep up cruft I can find in my assigned distribution for this month’s edition of the CPAN Pull Request Challenge.
This month’s module: Dist::Zilla::Plugin::GitHubREADME::Badge
Dist::Zilla::Plugin::GitHubREADME::Badge adds badges to GitHub’s README.md
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 22nd of September 2016 (https://github.com/fayland/perl-Dist-Zilla-Plugin-GitHubREADME-Badge)
- latest release 22nd of September 2016 (https://metacpan.org/release/Dist-Zilla-Plugin-GitHubREADME-Badge)
- issues are only tracked on GitHub, the RT queue points to the GitHub issues page as the preferred bugtracker.
- 1 open issue on GitHub
- was opened at the end of 2014 and had some discussion, however seems to be as yet unresolved
- 0 open pull requests
- 2 reverse dependencies via metacpan
- 10 test failures on cpantesters: http://www.cpantesters.org/distro/D/Dist-Zilla-Plugin-GitHubREADME-Badge.html
- most failures seem to occur on Windows
- failures on NetBSD and FreeBSD (one each) complain that there was no
plan in the TAP output for the
t/phase.t
test, however this test usesdone_testing
, so everything should be ok. Maybe specify the plan for the two subtests explicitly?
- CPANTS report (http://cpants.cpanauthors.org/dist/Dist-Zilla-Plugin-GitHubREADME-Badge)
- core issues:
- prereq_matches_use fixed in PR#21
- extra issues:
- meta_yml_declares_perl_version fixed in PR#18
- experimental issues:
- core issues:
The impression one gets is of a well cared-for distribution with maybe a few rough edges, however only in terms of kwalitee.
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.
- uses
Dist::Zilla
- authordeps include the module itself…
dzil authordeps --missing | cpanm
works okdzil test
runs ok and passes the testsperlver .
says the minimum version of perl is v5.8.5; although a commit mentions thatDist::Zilla
only supports from v5.14. The minimum Perl version was set in PR#17.- uses Travis-CI, however the list of Perls could be extended (fixed in PR#17).
The initial inspection doesn’t really provide much fodder for pull requests, there a couple of things which one might consider as low hanging fruit, however, this looks to be good stuff.
While reviewing the code a bit more closely, I noticed that Test::More
was
being explicitly installed in the Travis configuration file. This module is
part of the Perl core, so shouldn’t need to be installed (at least, not
explicitly). Hence I submitted PR#22 which removed the unnecessary
installation step.
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:
$ AUTHOR_TESTING=1 dzil cover
In this distribution, the coverage is:
98.2% total statement coverage; 91.7% total coverage.
which is excellent!
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 # 0 errors
$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep WARN # 0 warnings
Nothin’ to see here… Let’s see if anything else can be cleaned up.
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)
In this month’s module only one file was found with trailing whitespace,
namely: LICENSE
. This isn’t really important enough an issue to fix.
I got tripped up this month: there was trailing whitespace which was only
tabs, hence the grep
above needs to be extended like so:
$ vim $(git grep '\s\+$' | cut -d':' -f1 | sort | uniq)
which showed that there are two lines in the main library file with trailing whitespace. This issues was fixed in PR#23.
Add files to .gitignore
None required in this dist.
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
No problems found at the default perlcritic severity level.
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://'
Links look ok.
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)
The distribution uses an open-ended copyright statement, i.e. the copyright starts at a given year and the end year is not specified. Hence it doesn’t seem necessary to add an end year to the copyright statements.
Files missing an email address in copyright
$ vim $(git grep -i copyright | grep -P '\d{4}' | grep -v '@' | cut -d':' -f1 | uniq)
This distribution doesn’t use email addresses in its copyright strings, hence this update is unnecessary.
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.
In this month’s distribution only the one file needed to be checked, so that made the process simple. No spelling errors were found. Some minor grammatical and sentence flow issues were corrected in PR#16.
Kwalitee tests
Although CPANTS is the main kwalitee
reference, one can also run the kwalitee tests locally. One can use the
t/99kwalitee.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
To use the kwalitee tests in this month’s module, it was necessary to first
build the project with dzil build
, then enter the build directory
Dist-Zilla-Plugin-GitHubREADME-Badge-0.18
and then to run the tests via
prove
, however specifying the test file in the project directory. In
other words:
RELEASE_TESTING=1 prove -l ../t/99kwalitee.t
../t/99kwalitee.t .. 1/25
# Failed test 'meta_yml_declares_perl_version by Module::CPANTS::Kwalitee::MetaYML'
# at ../t/99kwalitee.t line 12.
# Detail: This distribution does not declare the minimum perl version in META.yml.
# Remedy: If you are using Build.PL define the {requires}{perl} = VERSION field. If you are using MakeMaker (Makefile.PL) you should upgrade ExtUtils::MakeMaker to 6.48 and use MIN_PERL_VERSION parameter. Perl::MinimumVersion can help you determine which version of Perl your module needs.
# Failed test 'prereq_matches_use by Test::Kwalitee::Extra'
# at ../t/99kwalitee.t line 12.
# Detail: This distribution uses a module or a dist that's not listed as a prerequisite.
# Detail: Missing: Moose in Moose, Moose::Util::TypeConstraints in Moose, namespace::autoclean in namespace-autoclean
# Remedy: List all used modules in META.yml requires
# Looks like you failed 2 tests of 25.
../t/99kwalitee.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/25 subtests
These errors are in essence a repetition of the warnings found on CPANTS mentioned in the “First impressions” section above.
Since the distribution uses
Dist::Milla
(a collection of
Dist::Zilla
plugin bundle, minting profile and a command line wrapper),
and as a result the dist.ini
is very simple. Although it would be
tempting to add the required prerequisites to [Prereqs]
section in
dist.ini
, this is the wrong thing to do with a Milla project; here one can
simply use the cpanfile
to define prereqs.
After having checked this distribution with a newly perlbrewed Perl, I found
that Test::Pod
was a missing test requirement. This was fixed in PR#19.
Since the patches to fix the core kwalitee issues were applied, I thought
I’d turn up the kwalitee a notch by dealing with the experimental issues as
well. This one can check locally by passing the :experimental
flag to
Test::Kwalitee::Extra
use Test::Kwalitee::Extra qw(:experimental);
This gave the following errors (the same as mentioned on the CPANTS page in the “Experimental issues” section):
# Failed test 'meta_yml_has_provides by Module::CPANTS::Kwalitee::MetaYML'
# at ../t/99kwalitee.t line 12.
# Detail: This distribution does not have a list of provided modules defined in META.yml.
# Remedy: Add all modules contained in this distribution to the META.yml field 'provides'. Module::Build or Dist::Zilla::Plugin::MetaProvides do this automatically for you.
# Failed test 'build_prereq_matches_use by Test::Kwalitee::Extra'
# at ../t/99kwalitee.t line 12.
# Detail: This distribution uses a module or a dist in its test suite that's not listed as a build prerequisite.
# Detail: Missing: warnings in Apache-Test
# Remedy: List all modules used in the test suite in META.yml build_requires
A fix for the missing warnings
package was submitted in PR#24, and a fix
for the missing provides
field was submitted in PR#25.
GitHub issues
The only open issue is from the end of 2014, has been well discussed and its resolution depends upon the interested parties replying to a question in the issue. Other than that, there’s nothing more in this distribution issue-wise to investigate.
RT issues
The RT queue points to the GitHub issues page.
Overview of the pull requests made
- Fix minor grammar and sentence flow issues in POD
- merged!
- Extend Perl versions tested on Travis
- merged!
- Declare minimum Perl version
- merged!
- Add Test::Pod test dependency
- merged!
- Add missing prereqs to match use
- closed due to merge conflicts which were easier to resolve by making a new pull request
- Add missing prereqs to match use
- replaces PR#20
- merged!
- Remove Test::More from Travis config
- merged!
- Purge trailing whitespace
- merged!
- Add prereq missing from build_requires
- merged!
- Generate “provides” info in META files
- merged!
- Add warnings pragma to main package
- Add shebang line to test scripts
- Make PhaseReadme test package a package
Conclusion
Other than kwalitee issues, there wasn’t much to dig into this month. It was already a project with good base code quality (wow! Did you see the code coverage??), and hence didn’t need much work to polish off the few rough edges.
Many thanks to FAYLAND for merging the pull requests! It’s nice to see changes being merged and so quickly! A new version of the module has also been released (version 0.20), which is really cool; simply awesome to have such fast turn around.