Ævar Arnfjörð Bjarmason wrote:
> Change the two wrappers to load from CPAN (local OS) or our own copy
> to do so via the same codepath.
nit: I think with s/to load/that load/ this will be easier to read.
> I added the Error.pm wrapper in 20d2a30f8f ("Makefile: replace
> perl/Makefile.PL with simple make rules", 2017-12-10), and shortly
> afterwards Matthieu Moy added a wrapper for Mail::Address in
> bd869f67b9 ("send-email: add and use a local copy of Mail::Address",
> 2018-01-05).
>
> His was simpler since Mail::Address doesn't have an "import" method,
> but didn't do the same sanity checking, e.g. a missing FromCPAN
> directory (which OS packages are likely not to have) wouldn't be
> explicitly warned about.
I'm having trouble parsing this. Mail::Address didn't do the same
sanity checking or his didn't?
The comma before e.g. should be a period or semicolon, since it's
starting a new independent clause.
> Now both use a modification of the previously Error.pm-specific
> codepath, which has been amended to take the module to load as
> parameter, as well as whether or not that module has an import method.
Does "now" mean before this patch or after this patch? Usually
commit messages describe the status quo without the patch in the
present tense and the change the patch will make in the imperative.
So this could say:
Update both to use a common implementation based on the previous
Error.pm loader.
[...]
> +++ b/perl/Git/LoadCPAN.pm
> @@ -0,0 +1,74 @@
[...]
> +The Perl code in Git depends on some modules from the CPAN, but we
> +don't want to make those a hard requirement for anyone building from
> +source.
not about this patch: have we considered making it a hard requirement?
Both Mail::Address and Error.pm are pretty widely available, and I
wonder if we could make the instructions in the INSTALL file say that
they are required dependencies to simplify things.
I admit part of my bias here is coming from the distro world, where we
have to do extra work to get rid of the FromCPAN embedded copies and
would be happier not to have to.
[...]
> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
> +preferring to use their own packaging of CPAN modules instead.
nit: I think the plural of OS is OSes, or something like
"distributors" or "operating systems".
[...]
> + eval {
> + require $package_pm;
> + 1;
> + } or do {
also not about this patch: this mixed tabs/spacing formatting feels a
bit unusual. I don't know if it's idiomatic for perl, and if it is
then no complaints; it just stood out a little. Can
Documentation/CodingGuidelines say something about the preferred
indentation in Perl to avoid having to think about such questions?
> --- a/perl/Git/LoadCPAN/Error.pm
> +++ b/perl/Git/LoadCPAN/Error.pm
> @@ -2,45 +2,9 @@ package Git::LoadCPAN::Error;
> use 5.008;
> use strict;
> use warnings;
> +use Git::LoadCPAN (
> + module => 'Error',
> + import => 1,
> +);
Nice!
Thanks and hope that helps,
Jonathan
