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.
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::Zillato 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.
dzil authordeps --missing | cpanmworked as expected.
dzil listdeps --author --missing | cpanmunfortunately 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::Zillaplugin bundle on my system. Therefore, I tried uninstalling
Dist::Zilla::PluginBundle::Author::DOMM, however that didn’t help.
- Adding the
copyright_holderfields to the
dist.inifile allowed the
dzil listdepscommand to work.
- I remember seeing an issue like this in another project and the reason was
a change in
Dist::Zillawhich was causing the problems.
- Digging further, I found that the installation issues were caused by warnings
dzil listdepsbeing piped into
cpanm. Fixing the warnings thus allowed
cpanmto work properly and install all of the required dependencies.
- As a quick check, I made sure that
dzil testran 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 testran as expected and the tests pass.
Now we can start looking for things that might need fixing or improving.
As mentioned above, the issue with the
listdeps failures was due to
Dist::Zilla producing warnings which were then being piped into
copyright_holder fields were added appropriately,
the failures disappeared and the project could be built.
While reading the documentation I noticed some minor issues in the wording and some unnecessary vertical whitespace (among other things).
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.
According to CPANTS (a
“kwalitee” testing service for
CPAN modules), the
META.yml file is missing. Hence, I added the file by
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
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
I wasn’t aware of this point (I thought one had to have both
files), so it was good to learn something new!
Status: closed unmerged
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
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.
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.
If you like what I do and want to spur me on to submit even more PRs, please buy me a coffee!