Pull Request Club: April 2021
The Pull Request Club is a way to connect open source maintainers to contributors through monthly assignments. It’s free to join up (just use your GitHub login) and it’s free to take part. At the start of each month members receive an email with a link to the GitHub repository of their assigned project. They then have one month to submit at least one pull request (PR) to this project. One can of course submit more than one pull request! The project is built to be a successor of the CPAN Pull Request Challenge (which was specific to the Perl community) but gives members the chance to contribute to a much wider variety of programming languages.
Each month, I make notes about the assignment I get, what I discovered and the PRs that I submitted.
This month I was assigned the Perl distribution CtrlO::Crypt::XkcdPassword.
Overview
After forking the project on GitHub and cloning the repository to my local computer I had a quick look at the code to orient myself and get an idea of what might need to be done.
- The project generates passwords along the lines of that outlined in the xkcd comic “Password Strength”.
- The distribution uses
Dist::Zilla
to build and test the code. - It seems to be well documented and has a comprehensive test suite.
Building the project
Before we can do anything else, it’s necessary to ensure that the project can build.
Installing the dependencies
Dist::Zilla
requires that the author and distribution dependencies are
installed before one can check that the test suite runs etc.
- Running
dzil authordeps --missing | cpanm
worked as expected. - Running
dzil listdeps --author --missing | cpanm
unfortunately failed. The error message started with:
[DZ] no license data in config, no %Rights stash, couldn't make a good guess
at license from Pod; giving up. Perhaps you need to set up a global config
file (dzil setup)? at inline delegation in Dist::Zilla for logger->log_fatal
(attribute declared in
/home/cochrane/perl5/perlbrew/perls/perl-5.30.1/lib/site_perl/5.30.1/Dist/Zilla.pm
at line 773) line 18.
! Finding [DZ] on cpanmetadb failed.
! Finding [DZ] () on mirror http://www.cpan.org failed.
! Couldn't find module or a distribution [DZ]
- I need to work out what this problem is first.
Fixing the ‘no license data in config’ error
- This issue could be caused by an old version of the author’s
Dist::Zilla
plugin bundle on my system. Therefore, I tried uninstallingDist::Zilla::PluginBundle::Author::DOMM
, however that didn’t help. - Adding the
license
andcopyright_holder
fields to thedist.ini
file allowed thedzil listdeps
command to work. - I remember seeing an issue like this in another project and the reason was
a change in
Dist::Zilla
which was causing the problems. - Digging further, I found that the installation issues were caused by warnings
from
dzil listdeps
being piped intocpanm
. Fixing the warnings thus allowedcpanm
to work properly and install all of the required dependencies. - As a quick check, I made sure that
dzil test
ran and that the tests pass. They did, so submitting this fix was my first PR this month.
Running the tests
Making sure that the test suite runs and that all tests pass ensures that
any code changes that come after this are covered by tests and thus start
from a solid foundation. With the known fix for the build failures seen
above, it was now just a matter of running dzil test
.
- Running
dzil test
ran as expected and the tests pass.
Now we can start looking for things that might need fixing or improving.
Pull Requests
Extend dist.ini attributes to fix listdeps failures
As mentioned above, the issue with the listdeps
failures was due to
Dist::Zilla
producing warnings which were then being piped into cpanm
.
After the license
and copyright_holder
fields were added appropriately,
the failures disappeared and the project could be built.
Status: merged
Fixed minor issues in docs
While reading the documentation I noticed some minor issues in the wording and some unnecessary vertical whitespace (among other things).
Status: merged
Removed superfluous whitespace
Removing trailing whitespace is something that some projects see as really nit-picky and unnecessary, and other projects see it as a sign of quality and hence needs to be removed (e.g. the Linux kernel). When I submit a patch like this I’m not worried if it gets accepted or not. It could be that this is something the author likes to have fixed; if not, that’s ok too! This PR removed the trailing whitespace found in the source code files.
Status: merged
Added META.yml
According to CPANTS (a
“kwalitee” testing service for
CPAN modules), the META.yml
file is missing. Hence, I added the file by
using the MetaYAML
Dist::Zilla
plugin. After a discussion with the
author, I found out that META.yml
is no longer necessary and should be
replaced by the META.json
file. In particular the documentation for
App::ModuleBuildTiny
states:
META.yml
This is the legacy meta file. This is mainly useful for bootstrapping on CPAN clients too old to support META.json but recent enough to support
configure_requires
.
I wasn’t aware of this point (I thought one had to have both META.*
files), so it was good to learn something new!
Status: closed unmerged
Set minimum Perl version
It can be helpful to set the minimum Perl version explicitly so that
downstream users of the code know which Perls can be used to install the
distribution. The module
Perl::MinimumVersion
has a
command perlver
which one can use to find out the appropriate version of
Perl to set as the explicit minimum version. Adding use 5.<xyz>
to the
library source files was all it took to set the minimum Perl version.
Status: merged
Add GitHub Actions configuration
It’s handy to have a continuous integration (CI) system build a project’s code and run its test suite automatically so that any problems not picked up in local development and testing can be spotted and corrected as appropriate. GitHub has its own CI service called GitHub Actions. This pull request added a simple workflow configuration file which builds the dist and runs the test suite on each push to the repository as well as on any pull request.
Status: merged
Support
If you like what I do and want to spur me on to submit even more PRs, please buy me a coffee!
