search

ssimms / pdfapi2

Create, modify, and examine PDF files in Perl
Other
15 stars 20 forks source link

Add missing prereq to dist.ini #12

Closed paultcochrane closed 7 years ago

paultcochrane commented 7 years ago

The module Win32::TieRegistry was found by CPANTS to be missing from the list of prerequisites for the module, although it is used. Adding the prereq fixes the prereq_matches_use core kwalitee issue (as tested via Test::Kwalitee::Extra).

This PR is intended to be helpful. If it needs to be changed in any way, please just let me know and I'll update it and resubmit.

ssimms commented 7 years ago

Win32::TieRegistry is only needed on Win32 platforms, so including it as a universal prerequisite doesn't seem like the right thing to do. Is there a way to limit it to that platform?

paultcochrane commented 7 years ago

You're quite right, that was daft of me... I'll see if I can limit the prerequisite and will resubmit the PR.

paultcochrane commented 7 years ago

The best bet seems to be to use the OSPrereqs plugin. It patches the Makefile.PL and adds Win32::TieRegistry to the list of prereqs if the OS is Win32. The downside is that CPANTS will still complain about the missing prerequisite. The upside is that Windows users will be able to install all dependencies without having to first run into the "oh, I need to install Win32::TieRegistry so that my code works" problem; it basically works transparently for them and the other platforms won't notice. What do you think? I can change my commit and do a force push so that you only have to merge the one commit.

ssimms commented 7 years ago

Ok, I think that makes this the same as pull request #1, which I just merged, so I'll close this one. Thanks for looking into it, and it'll be good to eliminate a potential stumbling block for Windows users.