… 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: MojoX::Transaction::WebSocket76
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 28th Jan 2013
- 0 pull requests outstanding, 2 closed
- 1 open issues in GitHub
- latest release: 28th of Jan, 2013 (https://metacpan.org/release/MojoX-Transaction-WebSocket76)
- 1 reverse dependency via metacpan
- CPAN testers failures (http://www.cpantesters.org/distro/M/MojoX-Transaction-WebSocket76.html)
- CPANTS warnings: http://cpants.cpanauthors.org/dist/MojoX-Transaction-WebSocket76
- Core and Extra metrics all pass
- Experimental metrics
- META.yml needs provides
- has separate license file
- build prereq matches use
This seems to be sort of a niche module and hasn’t been updated in a while, the main issue seems to be that it doesn’t work with current Mojolicious versions. Other than that, there are some cosmetic fixups one could undertake.
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.
- convert the
cpanm --installdeps .necessary for dependencies
- also doesn’t use coveralls
- according to
perlver, the minimum Perl version is 5.10.1, however this is explicit only in Makefile.PL and the rest is implicitly 5.8.0. This is actually due to the Mojolicious dependency: Mojolicious depends upon
- it turns out that
DEBUGetc. in version 6+ of Mojolicious
- with Mojolicious <= 5.17, the tests pass
- … which means that a requirement is
TEST_POD=1in order to test pod coverage. Gives 3 naked subroutines:
RELEASE_TESTING=1to check the
Test::CheckManifestin order to run this test
T::CMis installed, the test complains about a lot of files which could be skipped. -> add to
- convert hard tabs to spaces? -> consistently indented with tabs, no need to convert as it looks like the author has this style
- need to add a
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:
23.8% statement coverage; 22.3% total coverage
… which is unfortunately somewhat low. One could look into increasing test coverage.
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:
So nothing which needs fixing, yay!
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
No files need to be fixed.
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 warnings
- expression form of
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.
They all look ok.
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.
Things look good.
- 1 to look into
- it mentions that one can’t use the module with Mojolicious 6.40 and
later. This is because some constants from Mojolicious, needed in
MojoX::Transaction::WebSocket76, have been removed.
- a fix for this issue was submitted in PR#12.
- it mentions that one can’t use the module with Mojolicious 6.40 and later. This is because some constants from Mojolicious, needed in
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://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
Core and extras kwalitee tests pass.
Overview of the pull requests made
- skip more files from MANIFEST - https://github.com/dionys/mojox-transaction-websocket76/pull/5
- update copyright year - https://github.com/dionys/mojox-transaction-websocket76/pull/6
- add docs for overridden methods - https://github.com/dionys/mojox-transaction-websocket76/pull/7
- quieten perlcritic for unnecessary strict/warnings errors - https://github.com/dionys/mojox-transaction-websocket76/pull/8
- add license file - https://github.com/dionys/mojox-transaction-websocket76/pull/9
- convert README to markdown - https://github.com/dionys/mojox-transaction-websocket76/pull/10
- merged as part of another commit
- minor improvements to doc text - https://github.com/dionys/mojox-transaction-websocket76/pull/11
- fix with Mojolicious 6.4+ - https://github.com/dionys/mojox-transaction-websocket76/pull/12
Many thanks to DIONYS for merging my pull requests!
The author reacted quickly to pull requests, which was really nice. A new version of the dist was released to CPAN on the 16th of April 2016, yay!