fix: unfreeze pycountry version #22

aveuiller · 2018-07-04T09:43:13Z

This PR is mostly to fix the compatibility issue with pycountry (country_code being sometimes overridden). But I also tried to refactor the territory management and add a mapping of foreign territories.

I tried to understand and separated as well as possible the concepts you had behind the territories management. It's still far from perfect, but doesn't seem to break the behavior you intended to create. Tell me if it remains understandable on your side.

Please tell me if anything seems off in this PR since it has quite some changes ;)

Closes #16, #11

kdeldycke

I guess you can bump the component to 1.4.0.

kdeldycke · 2018-07-04T10:13:20Z

postal_address/territory.py

+    'DG': 'IO',  # Diego Garcia is part of the British Indian Ocean Territory
+    'FX': 'FR',  # France, Metropolitan
+    # European Commision country code exceptions.
+    # Source: http://publications.europa.eu/code/pdf/370000en.htm#pays


Link seems down. Tracked it down to: https://web.archive.org/web/20171108144027/http://publications.europa.eu/code/pdf/370000en.htm

http://publications.europa.eu/code/pdf/370000en.htm#pays Doesn't seem to be down for me

Did you tried https://publications.europa.eu/en/code/pdf/370000en.htm#pays ?

The one you sent here is not working (it is using https and with /en at the beginning). But the one in the code seems to work and has the same content as the web.archive.org link you sent me. Do you want to replace it regardless?

Aha! Got it! Just a missing https redirection! No no, keep the original URL! 😄

postal_address/territory.py

kdeldycke · 2018-07-04T10:16:22Z

postal_address/territory.py

-    'UK': 'GB',  # United Kingdom
-    'EL': 'GR',  # Greece
+    'UK': 'GB',  # United Kingdom is known as 'GB' in ISO-3166
+    'EL': 'GR',  # 'EL' is the european version of Greece,


Can you point to https://web.archive.org/web/20171108144027/http://publications.europa.eu/code/pdf/370000en.htm as the reference document that led to that decision?

postal_address/territory.py

kdeldycke · 2018-07-04T10:22:39Z

postal_address/territory.py

+    'FX': 'FR',  # France, Metropolitan
+    # European Commision country code exceptions.
+    # Source: http://publications.europa.eu/code/pdf/370000en.htm#pays
+    'EA': 'ES',  # 'EA' is the union of Ceuta and Melilla, Spanish territory


OK, agree on this one, let's make things simple.

kdeldycke · 2018-07-04T10:28:40Z

postal_address/territory.py

+    Map some subdivision aliases representing the same territory, but defined
+    under different countries.
+
+.. data:: FOREIGN_TERRITORIES_ALIAS_TO_COUNTRY


Why not rename that kind RESERVED_COUNTRY_CODES? As it it supposed to be an exact copy of special cases exposed at https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2#Reserved_code_elements right?

kdeldycke · 2018-07-04T10:51:10Z

postal_address/tests/test_territory.py

@@ -140,7 +146,7 @@ def test_alias_normalization(self):

        # Check country alias to a subdivision.
        self.assertEquals(
-            list(territory_parents_codes('TA')),
+            list(territory_parents_codes('SH-TA')),


Just to make sure I really follow you, what's the result of the previous version of the test (a.k.a. territory_parents_codes('TA'))?

Currently it would fail but I can fix it by adding a resolution against COUNTRY_ALIAS_TO_SUBDIVISION in territory_parents. This would send the same result for both queries. Would that be ok ?

Great! Sounds fantastic! Let's do that! :)

kdeldycke · 2018-07-04T11:02:53Z

postal_address/tests/test_territory.py

+        # Sub-territories will not change by default
+        self.assertEqual("BQ", normalize_territory_code("BQ"))
+        self.assertEqual("GP", normalize_territory_code("FR-GP"))
+        # TODO: Is it normal to not retrieve alpha2 ?


I guess this is allowed as the concept of a "territory" is covering both county and subdivisions. So to me, based on its naming normalize_territory_code() should return the most appropriate territory, be it a country or a subdivision.

kdeldycke · 2018-07-04T11:08:30Z

postal_address/tests/test_territory.py

+        self.assertEqual("FR", resolved)
+
+        resolved = normalize_territory_code("NL-BQ1",
+                                            resole_foreign_territory=True)


Seeing the usage of that parameter in the wild, I just realized that maybe resole_foreign_territory is badly named and should be called resolve_top_country or something. In which case it will not be allowed to return a subdivision but always a country.

kdeldycke · 2018-07-04T11:09:46Z

setup.py

@@ -31,7 +31,7 @@
    # TODO: subdivision definitions are broken for Czech Republic starting with
    # PyCountry 16.11.27. See:
    # https://bitbucket.org/flyingcircus/pycountry/issues/13389
-    'pycountry >= 16.11.08, < 16.11.27',
+    'pycountry >= 18.5.26',


I guess you can remove the TODO statement above then.

codecov-io · 2018-07-04T14:55:57Z

Codecov Report

Merging #22 into develop will increase coverage by 0.39%.
The diff coverage is 98.36%.

@@             Coverage Diff             @@
##           develop      #22      +/-   ##
===========================================
+ Coverage    95.61%   96.01%   +0.39%     
===========================================
  Files            3        2       -1     
  Lines          342      376      +34     
  Branches        91       95       +4     
===========================================
+ Hits           327      361      +34     
  Misses           5        5              
  Partials        10       10

Impacted Files	Coverage Δ
postal_address/address.py	`96.51% <100%> (+0.29%)`	⬆️
postal_address/territory.py	`94.91% <96.87%> (+0.97%)`	⬆️
postal_address/tests/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b48a43...5999562. Read the comment docs.

kdeldycke · 2018-07-06T13:27:51Z

postal_address/tests/test_territory.py

@@ -140,7 +146,7 @@ def test_alias_normalization(self):

        # Check country alias to a subdivision.
        self.assertEquals(
-            list(territory_parents_codes('TA')),
+            list(territory_parents_codes('SH-TA')),


Great! Sounds fantastic! Let's do that! :)

kdeldycke · 2018-07-06T14:05:08Z

postal_address/address.py

-        '{}_code'.format(subdiv_type_id): subdivision.code,
+        # Rename code to slug to avoid overriding 'country_code' in some cases
+        # See https://github.com/scaleway/postal-address/issues/16
+        '{}_slug'.format(subdiv_type_id): subdivision.code,


Now that I saw the use of that metadata in the wild, _slug might not be the right word, as it still holds a code. Don't you think _key, _area_code will be more appropriate here?

kdeldycke · 2018-09-10T13:22:38Z

Hey @aveuiller, what's the status of that PR? Do you want me to merge it?

aveuiller · 2018-09-10T13:44:49Z

Hello, yes this pull request is finished on my side. :)

kdeldycke · 2018-09-10T15:14:04Z

Let's merge then! Thanks @aveuiller for your patience and huge effort! 👍

Antoine Veuiller added 6 commits July 4, 2018 10:50

chores: update .gitignore

26db9b1

fix: unfreeze pycountry version

056949b

refactor(address): separate validate method into submethods

a932e1d

feat: add foreign territories mapping

5760e9b

refactor(territory): Separate country and subdivision mapping

8c3763d

style: resole pylint and pycodestyle issues

232bb7e

remyleone requested a review from kdeldycke July 4, 2018 10:17

kdeldycke suggested changes Jul 4, 2018

View reviewed changes

fix: apply review comments

e4e2419

Antoine Veuiller added 2 commits July 5, 2018 10:03

chores: bump version to 1.4.0 and update CHANGES.rst

1289683

fix(address): rename slug to area_code

5999562

kdeldycke reviewed Jul 6, 2018

View reviewed changes

kdeldycke approved these changes Sep 10, 2018

View reviewed changes

kdeldycke merged commit c309cf3 into scaleway:develop Sep 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unfreeze pycountry version #22

fix: unfreeze pycountry version #22

aveuiller commented Jul 4, 2018 •

edited

Loading

kdeldycke left a comment

kdeldycke Jul 4, 2018

aveuiller Jul 4, 2018

kdeldycke Jul 4, 2018

aveuiller Jul 4, 2018

kdeldycke Jul 6, 2018

kdeldycke Jul 4, 2018

kdeldycke Jul 4, 2018

kdeldycke Jul 4, 2018

kdeldycke Jul 4, 2018

aveuiller Jul 4, 2018

kdeldycke Jul 6, 2018

kdeldycke Jul 4, 2018

kdeldycke Jul 4, 2018

kdeldycke Jul 4, 2018

codecov-io commented Jul 4, 2018 •

edited

Loading

kdeldycke Jul 6, 2018

kdeldycke Jul 6, 2018

kdeldycke commented Sep 10, 2018

aveuiller commented Sep 10, 2018

kdeldycke commented Sep 10, 2018

fix: unfreeze pycountry version #22

fix: unfreeze pycountry version #22

Conversation

aveuiller commented Jul 4, 2018 • edited Loading

kdeldycke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 4, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdeldycke commented Sep 10, 2018

aveuiller commented Sep 10, 2018

kdeldycke commented Sep 10, 2018

aveuiller commented Jul 4, 2018 •

edited

Loading

codecov-io commented Jul 4, 2018 •

edited

Loading