… 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: DBIx::Class::InflateColumn::Serializer
DBIx::Class::InflateColumn::Serializer provides inflators to serialize data structures for DBIx::Class
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 29th of April 2016 (https://github.com/miquelruiz/DBIx-Class-InflateColumn-Serializer)
- latest release 29th of April 2016 (https://metacpan.org/release/DBIx-Class-InflateColumn-Serializer)
- 0 open issues on GitHub (https://github.com/miquelruiz/DBIx-Class-InflateColumn-Serializer/issues)
- 0 open pull requests (https://github.com/miquelruiz/DBIx-Class-InflateColumn-Serializer/pulls)
- 1 open ticket on RT (https://rt.cpan.org/Public/Dist/Display.html?Name=DBIx-Class-InflateColumn-Serializer)
- 8 reverse dependencies via metacpan
- 0 test failures on cpantesters: http://www.cpantesters.org/distro/D/DBIx-Class-InflateColumn-Serializer.html
- CPANTS report (http://cpants.cpanauthors.org/dist/DBIx-Class-InflateColumn-Serializer)
The initial impression is of stable software which just needs some rough edges smoothed off.
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.
- no Travis config file. Added in PR#5
dzil authordeps --missing | cpanmworks ok
dzil testfails on first run with errors such as
t/002_storable.t .. Base class package "DBIx::Class::Schema" is empty. (Perhaps you need to 'use' the module which defines that package first,
dzil listdeps --author --missing | cpanmto see if more deps are required. Yup, more deps are required.
- that did the trick, now
dzil testpasses as expected.
perlver .says the minimum version of perl is v5.8.0. Set in PR#6.
- stumbled across the dist’s own Kwalitee test:
xt/kwalitee.t. This test uses
Test::Kwalitee. Upon installation of
Test::Kwaliteeand building of the dist locally (
dzil build) I found that inside the built distribution directory, that the tests passed correctly (
perl xt/kwalitee.tran without error).
- TIL: if one installs the CheckExtraTests Dist::Zilla plugin, then it’s
possible to run the tests in the
xt/directory, one needs to run
dzil xtestpasses all the extra tests
The code seems to be well organised, well documented and well tested. I
managed to learn this month, that one really does need to use
--author in order to get all of the required dependencies, and that the
xtest command exists. Something else to add to the toolbox. It’s
difficult to see if any change I can make will have a big impact to this
dist, since there really only seem to be small “best practices” clean-ups to
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
Dist::Zilla projects, one needs to install the
Dist::Zilla::App::Command::cover plugin, after which the code coverage can
be checked via:
In this distribution, the coverage is:
100.0% total statement coverage; 95.2% total coverage.
which is excellent and can’t really be improved upon.
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.
podchecker gives the following errors and warnings:
… no POD errors.
… and no POD warnings.
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 '\s\+$'. It can
be helpful to load the files found directly into
The list of files to fix was:
Since the README is automatically generated there is a line with residual trailing whitespace since the code tries to indent a line without content, hence adding 4 spaces to an empty line. One can thus ignore this extra whitespace. The fix for this issue was submitted in PR#10.
Add files to .gitignore
No files need to be added to
Perl::Critic will show up many
potential issues for Perl code. By simply running
perlcritic on the
t directories, one can get a further handle on the code’s quality.
Code before strictures problems found in the standard test suite as well as the extra test suite. Fixed in PR#11.
A bareword filehandle was found as well as the two-argument form of
Fixed in PR#12.
There are also “expression form of
eval” and “
return statement with
undef” issues, however these don’t really seem to be large enough
problems to fix.
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.
Links look ok.
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
Found that the copyright year had been left at 2014; the current release is from 2016, however, so this was updated in PR#13.
Files missing an email address in copyright
This distribution doesn’t use email addresses in copyright statements.
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
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:
Now look for
.bak file and check differences between it and the output
.txt file, the process looks roughly like this:
Then update the appropriate
.pod files as necessary.
Spell checked files looked good.
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://ptc-it.de/cpan-pull-request-challenge/ for this purpose.
However, the script only uses
Test::Kwalitee which doesn’t cover as many
metrics as CPANTS.
set of metrics closer to that used on CPANTS, so replace the
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
distribution like so:
or, if the distribution uses
The errors picked up by the kwalitee tests were a repetition of those issues found on CPANTS. These issues have been addressed in already submitted PRs.
There are no open GitHub issues.
The single bug mentioned in the RT queue was resolved in PR#7, since it’s
related to the location of the prereqs in the
Overview of the pull requests made
- Add initial Travis-CI configuration
- Declare minimum Perl version explicitly
- Move prereqs into correct dist.ini section
- Create a META.json file when building the dist
- Add package provides info to dist META info files
- Purge trailing whitespace
- Add strict and warnings pragmas in test files
- Replace bareword filehandle with lexical variable
- Update copyright year to that of current release
As yet no reaction from the author concerning the PRs. However, it’s still the start of the month, so one can’t expect a reply so soon! Not sure if there’s anything else worthwhile I can help out with here. Perhaps if the author responds to the PRs I can ask if there’s anything burning issue which would also be of use to the distribution. We’ll just have to wait and see.
The author merged all of the PRs and released version 0.09 of the dist! Awesome!