shawnlaffan / Geo-ShapeFile

A perl library for reading shapefiles
https://metacpan.org/module/Geo::ShapeFile
2 stars 1 forks source link

Values of 0 and undef treated the same #23

Closed pajlpajl closed 5 years ago

pajlpajl commented 5 years ago

The method _get_shp_shx_headervalue incorrectly returns undef instead of 0. Values of 0 are valid for coordinates and occur, say, at the Greenwich meridian. The line below is incorrect: return $self->{'shx' . $val} || $self->{'shp' . $val} || undef; This line is correct: return $self->{'shx' . $val} // $self->{'shp' . $val} // undef; The line just a few lines above it if (!($self->{'shx' . $val} || $self->{'shp_' . $val})) { will call $self->_read_shx_header() needlessly a second or more times and would also be improved using defined().

I did not review whether anything similar occurs elsewhere in the code.

shawnlaffan commented 5 years ago

Thanks for reporting.

As with #22, a pull request with a test would be appreciated.

In terms of the defined-or operator, we currently support 5.8. That said, bumping the minimum perl to 5.10 is an open issue (see #2).

At the very least the trailing || undef could be removed as it is redundant.

pajlpajl commented 5 years ago

If you just remove the trailing undef, it may still be incorrect.

                return $self->{'shx_' . $val} || $self->{'shp_' . $val};

You may have $self->{'shx' . $val} validly set to zero, and if $self->{'shp' . $val} is not defined, you will get undef returned. And if you exchange shx <-> shp you get 0 returned. So the whole construct is not well-defined. The below would suffice I think:

            return defined($self->{'shx_' . $val}) ? $self->{'shx_' . $val} :  $self->{'shp_' . $val}

From: shawnlaffan notifications@github.com Sent: Sunday, February 10, 2019 5:05 PM To: shawnlaffan/Geo-ShapeFile Geo-ShapeFile@noreply.github.com Cc: Dr. Peter A. J. Liggatt pliggatt@microdecisions.com; Author author@noreply.github.com Subject: Re: [shawnlaffan/Geo-ShapeFile] Values of 0 and undef treated the same (#23)

Thanks for reporting.

As with #22https://github.com/shawnlaffan/Geo-ShapeFile/issues/22, a pull request with a test would be appreciated.

In terms of the defined-or operator, we currently support 5.8. That said, bumping the minimum perl to 5.10 is an open issue (see #2https://github.com/shawnlaffan/Geo-ShapeFile/issues/2).

At the very least the trailing || undef could be removed as it is redundant.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/shawnlaffan/Geo-ShapeFile/issues/23#issuecomment-462179827, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQsLifWXLcpgBmnMGIPKWu76A_zYy8G5ks5vMJeWgaJpZM4azKds.

shawnlaffan commented 5 years ago

[update - fixed incorrect cpan syntax]

A dev release is now on CPAN at https://metacpan.org/release/SLAFFAN/Geo-ShapeFile-2.65_001

Could you please test if this works? If it does, and cpan testers stays green, then I will upload a new version.

You might already know this, but you can test it locally without installing it using these steps (updating path separators as needed for your OS):

cpanm --look Geo::ShapeFile@2.65_001
perl Makefile.PL
gmake
gmake test

perl -Mblib \path\to\your_perl_script_or_app.pl

You can also call it from some other path using

perl -Mblib=\path\to\geo_shapefile_build_dir\blib your_perl_script_or_app.pl

Or you can just add the blib\lib and blib\auto folders to the PERL5LIB env var.

pajlpajl commented 5 years ago

This works for me now, thank you.

However, this edit at line 426:

                                if (!defined ($self->{'shx_' . $val} || $self->{'shp_' . $val})) {

may still not do what you want. For instance:

perl -e 'if (!defined( 0 || undef)) {print "Got here\n"}'

prints “Got here”, and if either value is 0, you do not need the if clause to execute.

                                if (!defined ($self->{'shx_' . $val}) && !defined($self->{'shp_' . $val})) {

or

                                unless (defined ($self->{'shx_' . $val}) || defined($self->{'shp_' . $val})) {

may be what you need.

From: shawnlaffan notifications@github.com Sent: Sunday, February 10, 2019 6:03 PM To: shawnlaffan/Geo-ShapeFile Geo-ShapeFile@noreply.github.com Cc: Dr. Peter A. J. Liggatt pliggatt@microdecisions.com; Author author@noreply.github.com Subject: Re: [shawnlaffan/Geo-ShapeFile] Values of 0 and undef treated the same (#23)

A dev release is now on CPAN at https://metacpan.org/release/SLAFFAN/Geo-ShapeFile-2.65_001

Could you please test if this works? If it does, and cpan testers stays green, then I will upload a new version.

You might already know this, but you can test it locally without installing it using these steps (updating path separators as needed for your OS):

cpanm --look Geo::ShapeFile::2.65_001

perl Makefile.PL

gmake

gmake test

perl -Mblib \path\to\your_perl_script_or_app.pl

You can also call it from some other path using

perl -Mblib=\path\to\geo_shapefile_build_dir\blib your_perl_script_or_app.pl

Or you can just add the blib\lib and blib\auto folders to the PERL5LIB env var.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/shawnlaffan/Geo-ShapeFile/issues/23#issuecomment-462189079, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQsLiXOOFanlKIAqZS_v3MlBeDBJ5fIcks5vMKU7gaJpZM4azKds.

shawnlaffan commented 5 years ago

version 2.66 is now on cpan: https://metacpan.org/release/SLAFFAN/Geo-ShapeFile-2.66

shawnlaffan commented 5 years ago

Marking as fixed.