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 uninstalling Dist::Zilla::PluginBundle::Author::DOMM, however that didn’t help.
• Adding the license and copyright_holder fields to the dist.ini file allowed the dzil 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 into cpanm. Fixing the warnings thus allowed cpanm 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!