maxmind / geoipupdate-legacy

GeoIP update client code
GNU General Public License v2.0
258 stars 63 forks source link

Issue 67: change dependency to IO::Uncompress::Gunzip from PerlIO::gzip #101

Closed zielot closed 6 years ago

zielot commented 6 years ago

My update addresses the changes indicated by zzqzzqzzq's comments from Nov 9, 2017 suggesting changes to fix issue 67

zielot commented 6 years ago

Hi Gregory,

It would be a shame if you were to get rid of the Perl script. The difference in complexity reminds me of why I stopped developing in C. However, the reason why we prefer the Perl script is simple: we are a Java shop and do not have a C development environment set up anywhere whereas Perl is ubiquitous. If you were to provide binaries or even better, rpms then this would not be an issue.

~ Justin

On Mon, Apr 30, 2018 at 10:41 AM Gregory Oschwald notifications@github.com wrote:

@oschwald requested changes on this pull request.

Thanks for the change. I had a couple of small comments.

We may be removing the Perl script in the future. I am happy to merge a fixed version of this until then, but may I ask why you prefer the Perl version to the C one?

In bin/geoipupdate-pureperl.pl https://github.com/maxmind/geoipupdate/pull/101#discussion_r185023644:

@@ -257,7 +257,7 @@ sub _gunzip_and_replace {

--- uncompress the gzip data

{ local $_;

  • open my $gin, '<:gzip', \$content or die $!;
  • my $gin = new IO::Uncompress::Gunzip( \$content ) or die $!;

The indirect object syntax is fragile. Please change this to IO::Uncompress::Gunzip->new instead.

In bin/geoipupdate-pureperl.pl https://github.com/maxmind/geoipupdate/pull/101#discussion_r185023709:

@@ -49,7 +49,7 @@ use Getopt::Std; use HTTP::Request::Common; use LWP::UserAgent; -use PerlIO::gzip; +use IO::Uncompress::Gunzip;

Please alpha sort these.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxmind/geoipupdate/pull/101#pullrequestreview-116332161, or mute the thread https://github.com/notifications/unsubscribe-auth/ACwsAnwUglWzX4t0gdQ0PrquKEhmfkp7ks5ttzDAgaJpZM4Tla6- . ​T​

oschwald commented 6 years ago

Thanks!