[pve-devel] [PATCH 3/3] Fix #1527: Use iso-codes country naming

[Thomas Lamprecht]

comments inline:

On 9/12/18 2:52 PM, Rhonda D'Vine wrote:
> The iso 3166-1 information in the iso-codes package seems to be more
> up to par than the one that was used from tzdata.
> 
> Signed-off-by: Rhonda D'Vine <rhonda at proxmox.com>
> ---
>  country.pl     | 18 ++++++++++++------
>  debian/control |  1 +
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/country.pl b/country.pl
> index 277fc34..b149b5b 100755
> --- a/country.pl
> +++ b/country.pl
> @@ -4,6 +4,7 @@ use strict;
>  use warnings;
>  
>  use PVE::Tools;
> +use JSON;
>  
>  # see also: http://en.wikipedia.org/wiki/Keyboard_layout
>  #
> @@ -14,14 +15,19 @@ use PVE::Tools;
>  
>  my $country = {};
>  
> -my $line;
> -open (TMP, "</usr/share/zoneinfo/iso3166.tab");
> -while (defined ($line = <TMP>)) {
> -    if ($line =~ m/^([A-Z][A-Z])\s+(.*\S)\s*$/) {
> -	$country->{lc($1)} = $2;
> -    }
> +open (TMP, "</usr/share/iso-codes/json/iso_3166-1.json");

you missed to change the comment (out of context, at start of script):
# country codes from: /usr/share/zoneinfo/iso3166.tab

> +my $json_data;
> +{
> +    local $/;
> +    $json_data = <TMP>;
>  }
>  close (TMP);

you also change how we read a file (with no mentioning of that in the
commit message). And we have helpers for reading/writing files in PVE::Tools,
known to be good handling various corner cases. As PVE::Tools is already included
I'd suggest doing something like:

my $raw = PVE::Tools::file_get_contents("/usr/share/zoneinfo/iso3166.tab", 128*1024);

if this needs change anyway.

> +$json_data = from_json($json_data);

variable use for different stuff, and json_data is a too generic name, one
has no idea what could be possible contained in this variable reading only
the name... Use something more telling like $iso_codes_raw for the raw file
and $iso_codes for the (JSON) parsed hash.
And maybe add a $country_codes = $iso_codes->{'3166-1'} variable, it's the only
thing in there and that would make code below a bit shorter/easier to read.

> +
> +foreach my $entry (@{$json_data->{'3166-1'}}) {
> +    $country->{lc($entry->{'alpha_2'})} = defined $entry->{'common_name'} ?
> +	$entry->{'common_name'} : $entry->{'name'};
> +}

could be also written as:

my $country = {
    map { lc($_->{alpha_2}) => $_->{common_name} // $_->{name} } @{$json_data->{'3166-1'}};
};

but, while map is in general faster on such things, I have no problem with the foreach.

But something like the following in the loop body:

my $code = lc($entry->{'alpha_2'});
$country->{$code} = $entry->{common_name} // $entry->{name};

is much easier and quicker to read, IMO.

>  
>  # we need mappings for X11, console, and kvm vnc
>  
> diff --git a/debian/control b/debian/control
> index 60bc8b0..7ab4515 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -3,6 +3,7 @@ Section: perl
>  Priority: optional
>  Maintainer: Proxmox Support Team <support at proxmox.com>
>  Build-Depends: debhelper (>= 9),
> +               iso-codes,
>                 libpve-common-perl,
>                 librsvg2-bin,
>                 perl (>= 5.10.0-19),
>