add iso3166-1 properties to GeoJSON

[missinglink]

As discussed in https://github.com/pelias/api/issues/1540 this PR adds iso3166-1 properties to the GeoJSON. Since this adds a new set of properties per feature, I wanted to solicit some feedback before we consider merging.

In particular I'm not super happy with the property names, they are very 'wordy', but they do make it quite clear what they are referring to 🤷‍♂️

Also worth noting the names contain a - which requires the use of brackets to access in JS.

properties.iso3166-1_alpha2
properties.iso3166-1_alpha3
properties.iso3166-1_numeric

cc/ @pelias/contributors

closes https://github.com/pelias/api/issues/1540

[Joxit]

Hi, yes the property names seems weird.

If it is known that alpha 3 is used, I suggest something like

properties.country_a2
properties.country_n
# or
properties.dependency_a2
properties.dependency_n

[orangejulius]

We should definitely add these properties, they are super helpful and cost us nothing to add :)

The hardest part will definitely be the name! I'm leaning towards @joxit's suggestions as they're nice and concise and will not require special handling when referencing from JS.

The name of the standard and its variations do not make this job easy, and being part of the public API this is hard to change once we've decided, so we should think about it for a bit.

[missinglink]

I like the simplicity of the suggestion from @Joxit but it still requires the user to have their own logic to cover dependencies.

I was hoping that there was just a single field and it would always be set and always the same key regardless of whether an address is in Puerto Rico or Paris.

[missinglink]

heya, I'd like to revisit this and get it merged.

I think we all agree that the names I proposed are terrible 😆 I like the ones @Joxit proposed better.

I would really prefer that users didn't have to have any logic to handle dependency placetypes any differently from country placetypes, does that sound like a good idea?

Some other thoughts:

• The 'numeric' code from the iso3166-1 standard is rarely used, we could drop that if we wanted to
• The colloquial name for these properties is "country code" and in fact that term is a misnomer since it doesn't always refer to a country, but also dependent territories.

So we could use country_code or cc as a replacement term for iso3166-1?

In that case we could have something like:

properties.cc2
properties.cc3

[missinglink]

Another option, which we had in the past actually, is to use the terms alpha-2 and alpha-3 which are the names of the two non-numeric sets of iso3166-1:

properties.alpha2
properties.alpha3

[missinglink]

One other option which I considered but I feel less comfortable with that the ones above is to have an object with the key iso3166:

properties.iso3166 = {
  alpha2: '',
  alpha3: '',
  numeric: 0,
}

[missinglink]

I've changed the properties to cc2 and cc3 since that's the only one which got a vote 😄

I'm taking this out of DRAFT and will merge imminently unless there's any final thoughts.

[Joxit]

I like cc2 and cc3, short and catchy :stuck_out_tongue:.

[missinglink]

I added one more commit which moves the properties up higher:

This is mainly to avoid them coming after the addendum which actually has a code comment about being the last property.

[orangejulius]

Cool, these will be good additions.

Personally, I'd vote for country_code2 and country_code3. Correct me if I'm wrong, but most of our other properties are mostly not abbreviated (something like country_a has an abbreviation but there is also an unabbreviated word, and then there is id/gid).

To me, that feels more consistent but let me know what everyone else thinks. Maybe we could do country_2 and country_3 as a compromise?

Nice touch on ensuring the property values are sorted well (nitpic: you can squash that second commit since the commit message for it does not need to go in the release notes).

@missinglink if you like country_code2 or even country_2, feel free to make that change and go ahead and merge. If you still prefer cc2 then just let me know, I'm not totally against it but might need a bit more convincing before I can give an enthusiastic "shipit" 😆

[missinglink]

Yeah actually, I think country_code will suffice (with the alpha2), this is the same behaviour as nominatim. Generally libraries seem to support the 2 char code, so that's by far the most important part of this feature.

If users want the alpha3, it's already available as properties.country_a || properties.dependency_a. While I admit this isn't the most convenient, it's also duplicative to repeat those fields in the response.

This doesn't look out of place amongst the other properties:

[orangejulius]

That works just fine :)

And we can always add country_code3 later if we choose to. Much easier than removing.

[missinglink]

OK cool, my inner nerd cringes at the misnomer but country_code seems to be the generally accepted term and I think most people don't think as deeply into it as me ;)

I'm really aiming for compatibility with things like https://www.countryflags.io/ and they're using the same term :country_code to refer to the iso3166-1 alpha2 code.

[missinglink]

I added some JS safety checking to avoid any potential bugs:

diff --git a/helper/geojsonify.js b/helper/geojsonify.js
index 2223c489..ef69a6f4 100644
--- a/helper/geojsonify.js
+++ b/helper/geojsonify.js
@@ -176,12 +176,13 @@ function computeBBox(geojson, geojsonExtentPoints) {
 function addISO3166PropsPerFeature(geojson) {
   geojson.features.forEach(feature => {
     let code = _.get(feature, 'properties.country_a') || _.get(feature, 'properties.dependency_a') || '';
-    if (!code.length){ return; }
+    if (!_.isString(code) || _.isEmpty(code)){ return; }

     let info = iso3166.info(code);
-    if (_.isEmpty(info)) { return; }
+    let alpha2 = _.get(info, 'alpha2');
+    if (!_.isString(alpha2) || _.size(alpha2) !== 2) { return; }

-    _.set(feature, 'properties.country_code', info.alpha2);
+    _.set(feature, 'properties.country_code', alpha2);
   });
 }