Repository: kwzrd/pypopulation

Issue:
Load resource file at import time
The current methodology is to delay loading the resource file until the first lookup is made, and then to cache the loaded map using a 1-size LRU cache. This was done in an effort to make the import faster and to avoid the price of loading the file should the module be imported but never used. However, it adds some unnecessary complexity to both the implementation and the tests, and the advantages are debatable at best. Let's load the file at import time instead.

Discussion:

Patch:
diff --git a/README.md b/README.md
index b24bc4a..0fe7e4c 100644
--- a/README.md
+++ b/README.md
@@ -12,7 +12,7 @@ Lightweight population lookup using [ISO 3166](https://en.wikipedia.org/wiki/ISO
 83132799
 ```
 
-The aim is to provide a minimalist package with no dependencies that does one thing only, as best as possible. Population figures are read from a JSON file into Python dictionaries on the first lookup, _not_ at import time. The API then only exposes the dictionaries.
+The aim is to provide a minimalist package with no dependencies that does one thing only, as best as possible. Population figures are read from a JSON file into Python dictionaries at import time. The API then only exposes the dictionaries.
 
 **The given figures are estimates at best.** Read below for more details on the data source.
 
@@ -32,8 +32,6 @@ If you would like to build your own wrapper around the source JSON, you can do:
 countries: t.List[t.Dict] = pypopulation._load_file()
 ```
 
-**Note**: This function is wrapped in `functools.lru_cache(max_size=1)`. 
-
 ## Installation
 
 With `pip` from [PyPI](https://pypi.org/):
diff --git a/pypopulation/implementation.py b/pypopulation/implementation.py
index 7f514f1..5f82fb8 100644
--- a/pypopulation/implementation.py
+++ b/pypopulation/implementation.py
@@ -1,4 +1,3 @@
-import functools
 import json
 import typing as t
 from pathlib import Path
@@ -8,14 +7,12 @@ DATAFILE = Path(__file__).parent.joinpath("resources", "countries.json")
 PopulationMap = t.Dict[str, int]  # From country code to its population
 
 
-@functools.lru_cache(maxsize=1)
 def _load_file() -> t.List[t.Dict]:
     """Load `DATAFILE` into a Python list object."""
     with DATAFILE.open(mode="r", encoding="UTF-8") as datafile:
         return json.load(datafile)
 
 
-@functools.lru_cache(maxsize=1)
 def _initialize() -> t.Tuple[PopulationMap, PopulationMap]:
     """Init Alpha-2 and Alpha-3 maps from `DATAFILE`."""
     country_list = _load_file()
@@ -31,6 +28,12 @@ def _initialize() -> t.Tuple[PopulationMap, PopulationMap]:
     return alpha_2, alpha_3
 
 
+# The runtime maps get initialized the first time this module is imported,
+# which means that there is no overhead once a lookup is made, however it
+# slightly increases the cost of initial import
+_a2_map, _a3_map = _initialize()
+
+
 def _normalize(country_code: str) -> str:
     """Normalize `country_code` casing."""
     return country_code.upper()
@@ -51,8 +54,7 @@ def get_population_a2(country_code: str) -> t.Optional[int]:
 
     None if `country_code` does not exist in the map.
     """
-    a2_map, _ = _initialize()
-    return a2_map.get(_normalize(country_code))
+    return _a2_map.get(_normalize(country_code))
 
 
 def get_population_a3(country_code: str) -> t.Optional[int]:
@@ -61,5 +63,4 @@ def get_population_a3(country_code: str) -> t.Optional[int]:
 
     None if `country_code` does not exist in the map.
     """
-    _, a3_map = _initialize()
-    return a3_map.get(_normalize(country_code))
+    return _a3_map.get(_normalize(country_code))


Test patch:
diff --git a/tests/test_implementation.py b/tests/test_implementation.py
index 9d2feec..913c02f 100644
--- a/tests/test_implementation.py
+++ b/tests/test_implementation.py
@@ -1,154 +1,91 @@
-import typing as t
 import unittest
-from unittest.mock import MagicMock, patch
+from unittest.mock import patch
 
 from pypopulation import implementation as imp
 
-mock_a2_map = {"AA": 1}
-mock_a3_map = {"BBB": 2}
 
-mock_initialize = MagicMock(return_value=(mock_a2_map, mock_a3_map))
+class TestImplementationHelpers(unittest.TestCase):
+    """
+    Test private helpers in the implementation module.
 
-
-class TestImplementation(unittest.TestCase):
-    """Test the implementation module."""
-
-    def setUp(self) -> None:
-        """
-        Clear LRU caches.
-
-        Ensure that nothing is cached before each test.
-        """
-        imp._load_file.cache_clear()
-        imp._initialize.cache_clear()
-
-    # region: resource file
+    In short, this class tests private methods using the production resources file,
+    not mocked data. This therefore also makes weak assertions about the resources.
+    """
 
     def test_file_exists(self):
-        """Resource file exists and lives where expected."""
+        """Resource path exists and leads to a file."""
         self.assertTrue(imp.DATAFILE.exists())
         self.assertTrue(imp.DATAFILE.is_file())
 
-    def test_file_loads(self):
-        """Resource file is valid JSON and loads into a Python list."""
-        obj = imp._load_file()
-        self.assertIsInstance(obj, list)
+    def test_load_file(self):
+        """Resource file loads into a Python list of dicts."""
+        loaded = imp._load_file()
+        self.assertIsInstance(loaded, list)
+        for expected_dict in loaded:
+            self.assertIsInstance(expected_dict, dict)
 
-    def test_file_is_cached(self):
-        """Resource file is cached after first load."""
-        obj_a = imp._load_file()
-        obj_b = imp._load_file()
-        self.assertIs(obj_a, obj_b)
-
-    # endregion
-    # region: map initialization
-
-    def test_map_init(self):
-        """Country maps initialize from the resource file."""
+    def test_initialize(self):
+        """Initialize produces two mappings as expected."""
         a2_map, a3_map = imp._initialize()
-        self.assertIsInstance(a2_map, dict)
-        self.assertIsInstance(a3_map, dict)
-
-    def test_map_is_cached(self):
-        """Country maps are cached and do not re-build on re-query."""
-        a2_map_a, a3_map_a = imp._initialize()
-        a2_map_b, a3_map_b = imp._initialize()
-        self.assertIs(a2_map_a, a2_map_b)
-        self.assertIs(a3_map_a, a3_map_b)
 
-    # endregion
-    # region: lookup methodology (on mock data)
+        def check(dct, key_length):
+            """Perform a series of checks on `dct`."""
+            for key, value in dct.items():
+                self.assertIsInstance(key, str)  # keys must be strings
+                self.assertIsInstance(value, int)  # Values must be integers
+                self.assertEqual(len(key), key_length)  # Keys must be exactly `key_length` long
+                self.assertTrue(key.isupper())  # Keys must be strictly upper-cased
 
-    def check_pairs(self, pairs: t.Iterable[t.Tuple], func: t.Callable):
-        """Run `pairs` of input, expected output and compare them against `func` result."""
-        for code, expected_population in pairs:
-            with self.subTest(code=code, expected_population=expected_population):
-                self.assertEqual(expected_population, func(code))
+        check(a2_map, 2)
+        check(a3_map, 3)
 
     def test_normalize(self):
-        """The `_normalize_` functions makes all strings uppercase."""
-        pairs = [("", ""), (" ", " "), ("a", "A"), ("A", "A"), ("aBc", "ABC")]
-        self.check_pairs(pairs, imp._normalize)
-
-    def test_general_lookup_not_a_country(self):
-        """Check both maps and return None when queried code does not exist."""
-        patch_a2 = patch("pypopulation.implementation.get_population_a2", MagicMock(return_value=None))
-        patch_a3 = patch("pypopulation.implementation.get_population_a3", MagicMock(return_value=None))
-
-        with patch_a2 as mock_a2, patch_a3 as mock_a3:
-            out_value = imp.get_population("not_a_country")
-
-        self.assertIsNone(out_value)
-        mock_a2.assert_called_once_with("not_a_country")
-        mock_a3.assert_called_once_with("not_a_country")
-
-    def test_general_lookup_ask_a2_a3(self):
-        """Check both maps and return population when queried code exists in a3 map."""
-        patch_a2 = patch("pypopulation.implementation.get_population_a2", MagicMock(return_value=None))
-        patch_a3 = patch("pypopulation.implementation.get_population_a3", MagicMock(return_value=1234))
-
-        with patch_a2 as mock_a2, patch_a3 as mock_a3:
-            out_value = imp.get_population("alpha_3")
-
-        self.assertEqual(out_value, 1234)
-        mock_a2.assert_called_once_with("alpha_3")
-        mock_a3.assert_called_once_with("alpha_3")
-
-    def test_general_lookup_ask_a2_only(self):
-        """Check first map only and return population when queried code exists in a2 map."""
-        patch_a2 = patch("pypopulation.implementation.get_population_a2", MagicMock(return_value=1234))
-        patch_a3 = patch("pypopulation.implementation.get_population_a3", MagicMock(return_value=5678))
-
-        with patch_a2 as mock_a2, patch_a3 as mock_a3:
-            out_value = imp.get_population("alpha_2")
-
-        self.assertEqual(out_value, 1234)
-        mock_a2.assert_called_once_with("alpha_2")
-        mock_a3.assert_not_called()  # In this case, the A3 map is never called
-
-    @patch("pypopulation.implementation._initialize", mock_initialize)
-    def test_alpha_2_lookup(self):
-        """Find populations for 'AA' but not 'BBB' using `get_population_a2`."""
-        none_pairs = [(code, None) for code in ("", "a", "A", "b", "B")]
-        good_pairs = [(code, 1) for code in ("aa", "aA", "AA")]
-        self.check_pairs(none_pairs + good_pairs, imp.get_population_a2)
-
-    @patch("pypopulation.implementation._initialize", mock_initialize)
-    def test_alpha_3_lookup(self):
-        """Find populations for 'BBB' but not 'AA' using `get_population_a3`."""
-        none_pairs = [(code, None) for code in ("", "a", "A", "b", "B")]
-        good_pairs = [(code, 2) for code in ("bbb", "bbB", "BBB")]
-        self.check_pairs(none_pairs + good_pairs, imp.get_population_a3)
-
-    # endregion
-    # region: lookups on the actual data
-
-    def check_type(self, codes: t.Iterable[str], type_: t.Any, func: t.Callable):
-        """Check that all `codes` produce an instance of `type_` when passed to `func`."""
-        for country_code in codes:
-            with self.subTest(country_code=country_code, expected_type=type_):
-                self.assertIsInstance(func(country_code), type_)
-
-    def check_values(self, codes: t.Iterable[str], func: t.Callable):
-        """Check that all `codes` produce the same result when passed to `func`."""
-        values = set(func(code) for code in codes)
-        self.assertEqual(len(values), 1)
-
-    def test_germany(self):
-        """The same integer is returned for alternatives of Germany's country code."""
-        a2_codes = ["de", "DE", "dE"]
-        self.check_type(a2_codes, int, imp.get_population_a2)
-        self.check_values(a2_codes, imp.get_population_a2)
-
-        a3_codes = ["deu", "DEU", "dEu"]
-        self.check_type(a3_codes, int, imp.get_population_a3)
-        self.check_values(a3_codes, imp.get_population_a3)
-
-        all_codes = a2_codes + a3_codes
-        self.check_type(all_codes, int, imp.get_population)
-        self.check_values(all_codes, imp.get_population)
-
-    def test_non_country(self):
-        """None is returned for non-existing countries."""
-        non_country_codes = "abcde", "", "e"
-        self.check_type(non_country_codes, type(None), imp.get_population)
+        """Normalization returns uppercase strings."""
+        cases = [
+            ("", ""),
+            (" ", " "),
+            ("a", "A"),
+            ("1a", "1A"),
+            ("aAa", "AAA"),
+        ]
+        for before, after in cases:
+            self.assertEqual(imp._normalize(before), after)
+
+
+@patch("pypopulation.implementation._a2_map", {"AA": 1})
+@patch("pypopulation.implementation._a3_map", {"BBB": 2})
+class TestImplementationLookups(unittest.TestCase):
+    """
+    Test public API lookup methods against mocked data.
+
+    This class contains test for the public functions that expose the internal data.
+    All cases are ran against mocked data. These tests are completely disconnected
+    from the resource file that is used in production.
+    """
+
+    def test_get_population(self):
+        """Get population fetches population for both A2 and A3 codes."""
+        self.assertEqual(imp.get_population("AA"), 1)
+        self.assertEqual(imp.get_population("aa"), 1)
+        self.assertEqual(imp.get_population("BBB"), 2)
+        self.assertEqual(imp.get_population("bbb"), 2)
+        self.assertEqual(imp.get_population("CCC"), None)
+        self.assertEqual(imp.get_population("ccc"), None)
+
+    def test_get_population_a2(self):
+        """Get population A2 fetches population A2 codes only."""
+        self.assertEqual(imp.get_population_a2("AA"), 1)
+        self.assertEqual(imp.get_population_a2("aa"), 1)
+        self.assertEqual(imp.get_population_a2("BBB"), None)
+        self.assertEqual(imp.get_population_a2("bbb"), None)
+        self.assertEqual(imp.get_population_a2("CCC"), None)
+        self.assertEqual(imp.get_population_a2("ccc"), None)
+
+    def test_get_population_a3(self):
+        """Get population A2 fetches population A2 codes only."""
+        self.assertEqual(imp.get_population_a3("AA"), None)
+        self.assertEqual(imp.get_population_a3("aa"), None)
+        self.assertEqual(imp.get_population_a3("BBB"), 2)
+        self.assertEqual(imp.get_population_a3("bbb"), 2)
+        self.assertEqual(imp.get_population_a3("CCC"), None)
+        self.assertEqual(imp.get_population_a3("ccc"), None)