Fix minor issues #6
Conversation
|
Thank you for this! Please could you instead:
|
|
I've allowed the CI, which will fail as PDL now needs 5.14+ so don't worry about that. I'm about to update the CI config, so please rebase this past that. |
Implemented the suggested changes and rebased it past your changes to the CI yml file |
|
@a-shahba Thanks for accepting those suggestions. I see that you absolutely haven't rebased, but instead merged (a big clue is the word "merge" in one of the new commits). Please could you actually rebase, and force-push? |
Makefile.PL
Outdated
| @@ -183,7 +183,7 @@ WriteMakefile( | |||
| # 'TEST_REQUIRES' => { PDL => '2.004' }, | |||
| 'PREREQ_PM' => { PDL => '2.004' }, | |||
| 'LICENSE' => 'perl', | |||
| 'VERSION_FROM' => 'hdf5.pd', | |||
| 'VERSION_FROM' => 'HDF5.pm', | |||
There was a problem hiding this comment.
My suggestion here was not supposed to be literal. This shows that you need to just not change this, and delete the explicit version-setting in the metadata. What you're telling it to do here is read from a file that doesn't exist until it's created by running make, which won't work from a fresh checkout or CPAN distro, because this change breaks the perl Makefile.PL stage, which is why the CI is failing.
Makefile.PL
Outdated
| @@ -155,7 +155,7 @@ my $meta_merge = { | |||
| }, | |||
| }, | |||
| resources => { | |||
| license => [ 'http://cpansearch.perl.org/src/CHM/PDL-IO-HDF5-0.73/COPYRIGHT' ], | |||
| license => [ 'the same terms as Perl' ], | |||
There was a problem hiding this comment.
This was also not supposed to be literal. Please look in the CPAN::Meta::Spec for how to actually execute this.
Makefile.PL
Outdated
| @@ -115,6 +115,7 @@ $LIBS .= " -lz" if($zLib); | |||
| $LIBS .= " -ljpeg" if($jpegLib); | |||
| $LIBS .= " -lm"; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Please don't add random blank lines. However, deleting spurious whitespace from the end of lines as below is great!
a-shahba
force-pushed
the
fixMinorIssues
branch
from
1ed2335
to
74df90e
Compare
@mohawk2 I thought it would be enough for this PR to include your latest changes to the YAML file. I will rebase it shortly but I am curious why you prefer rebasing. I read your comments and hopefully I got your points correctly this time. If not, no worries! Let me know what needs to be modified and I will do it. |
Fixed two minor issues in the package
Dataset.pmdeclares%PDLtoHDF5fileMappingas a local variable, i.e.my %PDLtoHDF5fileMapping. However, this variable was already declared as a local one to the package (viaourstatement) at the top of the file. This will cause users' scripts with a shebang like#!/usr/bin/perl -wto print warning messages likeMakefile.PLis not consistent with the one inHDF5.pm. Because of this inconsistency when the user installs the package via cpanm (i.e.cpanm PDL::IO::[email protected]), incorrect package version will be embedded in the log message, which is misleading.