Repository: ansible/ansible-navigator (language: python)

Issue:
Rework KVS with types and extend MutableMapping instead of dict
Change:
- Make KVS subclass MutableMapping[str, str] instead of dict
- Add types to KVS
- Move KVS into first-class package and mount it in EEs differently
- Add unit tests for KVS
- Touch up pre-commit config for mypy (drop --ignore-missing-imports,
  and drop tests and docs dirs, add a few missing deps.

Test Plan:
- Ran pre-commit for both mypy versions
- New unit tests for KVS

Signed-off-by: Rick Elrod <rick@elrod.me>


Discussion / hints:

Fix patch:
diff --git a/.flake8 b/.flake8
index ed64127aa..dfe18bd43 100644
--- a/.flake8
+++ b/.flake8
@@ -117,7 +117,7 @@ per-file-ignores =
   src/ansible_navigator/ui_framework/ui.py: D200, D202, D205, D211, D400, D403, DAR101, DAR201, DAR401
   src/ansible_navigator/ui_framework/utils.py: D200, D205, D400, D403, DAR101, DAR201
   src/ansible_navigator/ui_framework/validators.py: D200, D400, D401, D403, DAR101, DAR201, DAR401
-  src/ansible_navigator/utils.py: D105, D200, D202, D205, D400, D401, D403, DAR101, DAR201, DAR401
+  src/ansible_navigator/utils/functions.py: D105, D200, D202, D205, D400, D401, D403, DAR101, DAR201, DAR401
   tests/__init__.py: D104
   tests/conftest.py: D210, D400, D401, D403, DAR101, DAR201, DAR301, DAR401
   tests/defaults.py: D200
@@ -217,9 +217,9 @@ per-file-ignores =
   tests/unit/test_cli.py: D200, D202, D205, D400, D403, DAR101, DAR201
   tests/unit/test_image_introspection.py: D200, D400, D403, DAR101, DAR201
   tests/unit/test_log_append.py: D200, D400, D403, DAR101
-  tests/unit/test_utils.py: D200, D202, D400, D403, DAR101
   tests/unit/test_yaml.py: D200
   tests/unit/ui_framework/test_colorize.py: D200, D205, D209, D400, DAR101
+  tests/unit/utils/test_functions.py: D200, D202, D400, D403, DAR101
 
 # Count the number of occurrences of each error/warning code and print a report:
 statistics = true
diff --git a/src/ansible_navigator/utils/key_value_store.py b/src/ansible_navigator/utils/key_value_store.py
index 09148967d..a5c21cb95 100644
--- a/src/ansible_navigator/utils/key_value_store.py
+++ b/src/ansible_navigator/utils/key_value_store.py
@@ -20,15 +20,15 @@
 
 
 class KVSKeysView(KeysView[str]):
-    """A glorified KeysView specific to, and returned by, methods in KeyValueStore"""
+    """A glorified KeysView specific to, and returned by, methods in KeyValueStore."""
 
 
 class KVSItemsView(ItemsView[str, str]):
-    """A glorified ItemsView specific to, and returned by, methods in KeyValueStore"""
+    """A glorified ItemsView specific to, and returned by, methods in KeyValueStore."""
 
 
 class KVSValuesView(ValuesView[str]):
-    """A glorified ValuesView specific to, and returned by, methods in KeyValueStore"""
+    """A glorified ValuesView specific to, and returned by, methods in KeyValueStore."""
 
 
 class KeyValueStore(MutableMapping[str, str]):
@@ -46,7 +46,10 @@ def __init__(self, filename: Union[str, Path]):
 
     @property
     def path(self) -> str:
-        """The filename where the KVS is stored on disk."""
+        """Provide the filename where the KVS is stored on disk.
+
+        :returns: The path to the key-value store
+        """
         return self._path
 
     def close(self) -> None:
@@ -55,7 +58,10 @@ def close(self) -> None:
         self.conn.close()
 
     def open(self) -> sqlite3.Connection:
-        """Establish the connection to the database."""
+        """Establish the connection to the database.
+
+        :returns: A connection to the database
+        """
         self.conn = sqlite3.connect(self.path)
         return self.conn
 
@@ -120,7 +126,7 @@ def items(self) -> KVSItemsView:
     # that dict's Mapping superclass wants. However, it's correct in our case.
     # Our sqlite table expects string keys (column type 'text'). It's an error to
     # pass something else. If we pass something too weird, the sqlite3 lib will
-    # throw an error at runrtime.
+    # throw an error at runtime.
     def __contains__(self, key: str) -> bool:  # type: ignore[override]
         """Check if a given key is in the key-value store.
 
@@ -175,10 +181,9 @@ def __iter__(self) -> Iterator[str]:
         return self.iterkeys()
 
     def __repr__(self) -> str:
-        """
-        KVS repr
+        """Represent the key-value store.
 
-        :returns: The KVS repr.
+        :returns: Representation of the key-value store
         """
         # Loosely based on collections.OrderedDict#__repr__
         if not self:


Test patch:
diff --git a/tests/unit/utils/__init__.py b/tests/unit/utils/__init__.py
new file mode 100644
index 000000000..ae22456e4
--- /dev/null
+++ b/tests/unit/utils/__init__.py
@@ -0,0 +1,1 @@
+"""Unit tests for the ansible-navigator utils."""
diff --git a/tests/unit/utils/conftest.py b/tests/unit/utils/conftest.py
index 6698536db..086f26788 100644
--- a/tests/unit/utils/conftest.py
+++ b/tests/unit/utils/conftest.py
@@ -1,7 +1,7 @@
-"""fixtures for share tests"""
+"""Fixtures for share tests."""
 
-import os
-import sys
+from pathlib import Path
+from typing import Generator
 
 import pytest
 
@@ -9,30 +9,30 @@
 
 
 @pytest.fixture
-def empty_kvs(tmp_path):
-    """
-    Gives us a temporary, empty KeyValueStore.
+def empty_kvs(tmp_path: Path) -> Generator[KeyValueStore, None, None]:
+    """Give us a temporary, empty KeyValueStore.
 
-    :returns: the temporary KVS
+    :param tmp_path: Path to a temporary directory
+    :yields: The temporary KVS
     """
-    db = tmp_path / "basic_kvs_usage.db"
-    yield KeyValueStore(db)
+    database_path = tmp_path / "basic_kvs_usage.db"
+    yield KeyValueStore(database_path)
 
 
 @pytest.fixture
-def kvs(tmp_path):
-    """
-    Gives us a temporary KeyValueStore with some data.
+def kvs(tmp_path: Path) -> Generator[KeyValueStore, None, None]:
+    """Give us a temporary KeyValueStore with some data.
 
-    :returns: the temporary KVS
+    :param tmp_path: Path to a temporary directory
+    :yields: The temporary KVS
     """
-    db = tmp_path / "basic_kvs_usage.db"
-    kvs = KeyValueStore(db)
-    kvs["banana"] = "blue"  # ewww
-    kvs["apple"] = "red"
-    kvs["grape"] = "green"
-    kvs["strawberry"] = "red"
-    kvs["banana"] = "yellow"  # just kidding
-    kvs["cherry"] = "red"
-    del kvs["cherry"]
-    yield kvs
+    database_path = tmp_path / "basic_kvs_usage.db"
+    key_value_store = KeyValueStore(database_path)
+    key_value_store["banana"] = "blue"  # yuck
+    key_value_store["apple"] = "red"
+    key_value_store["grape"] = "green"
+    key_value_store["strawberry"] = "red"
+    key_value_store["banana"] = "yellow"  # just kidding
+    key_value_store["cherry"] = "red"
+    del key_value_store["cherry"]
+    yield key_value_store
diff --git a/tests/unit/utils/test_key_value_store.py b/tests/unit/utils/test_key_value_store.py
index 6106f2897..6a484c313 100644
--- a/tests/unit/utils/test_key_value_store.py
+++ b/tests/unit/utils/test_key_value_store.py
@@ -1,34 +1,39 @@
-"""Test KVS from share.ansible_navigator."""
+"""Test KVS from ansible_navigator.utils."""
 
-import os
-import sys
 import types
 
-import pytest
-
 from ansible_navigator.utils.key_value_store import KeyValueStore
 
 
-def test_kvs_save_restore(empty_kvs):
-    """Test basic KVS write to disk and restore."""
-    empty_kvs["hello"] = "whooop"
+def test_kvs_save_restore(empty_kvs: KeyValueStore):
+    """Test basic KVS write to disk and restore.
+
+    :param empty_kvs: An empty key-value store
+    """
+    empty_kvs["hello"] = "whoop"
     empty_kvs["i_am_a_key"] = "and I am a value"
     empty_kvs.close()
 
     kvs2 = KeyValueStore(empty_kvs.path)
-    assert kvs2["hello"] == "whooop"
+    assert kvs2["hello"] == "whoop"
     assert kvs2["i_am_a_key"] == "and I am a value"
 
 
-def test_kvs_len(kvs):
-    """Test KVS __len__ / len()"""
+def test_kvs_len(kvs: KeyValueStore):
+    """Test KVS __len__ / len().
+
+    :param kvs: A key-value store populated with data
+    """
     assert len(kvs) == 4
     kvs["new_key"] = "something"
     assert len(kvs) == 5
 
 
-def test_kvs_iterkeys(kvs):
-    """Test KVS iterkeys()"""
+def test_kvs_iterkeys(kvs: KeyValueStore):
+    """Test KVS iterkeys().
+
+    :param kvs: A key-value store populated with data
+    """
     # Is it truly an iterator?
     assert isinstance(kvs.iterkeys(), types.GeneratorType)
 
@@ -37,8 +42,11 @@ def test_kvs_iterkeys(kvs):
     assert sorted(list(kvs.iterkeys())) == ["apple", "banana", "grape", "strawberry"]
 
 
-def test_kvs_itervalues(kvs):
-    """Test KVS itervalues()"""
+def test_kvs_itervalues(kvs: KeyValueStore):
+    """Test KVS itervalues().
+
+    :param kvs: A key-value store populated with data
+    """
     # Is it truly an iterator?
     assert isinstance(kvs.itervalues(), types.GeneratorType)
 
@@ -47,8 +55,11 @@ def test_kvs_itervalues(kvs):
     assert sorted(list(kvs.itervalues())) == ["green", "red", "red", "yellow"]
 
 
-def test_kvs_iteritems(kvs):
-    """Test KVS iteritems()"""
+def test_kvs_iteritems(kvs: KeyValueStore):
+    """Test KVS iteritems().
+
+    :param kvs: A key-value store populated with data
+    """
     # Is it truly an iterator?
     assert isinstance(kvs.iteritems(), types.GeneratorType)
 
@@ -65,8 +76,11 @@ def test_kvs_iteritems(kvs):
 # Now things get weird.
 
 
-def test_kvs_keys(kvs):
-    """Test KVS keys()"""
+def test_kvs_keys(kvs: KeyValueStore):
+    """Test KVS keys().
+
+    :param kvs: A key-value store populated with data
+    """
     # We get back a KVSKeysView which is just a collections.abc.KeysView in
     # disguise.
     keys = kvs.keys()
@@ -76,8 +90,11 @@ def test_kvs_keys(kvs):
     assert sorted(list(keys)) == ["apple", "banana", "grape", "strawberry"]
 
 
-def test_kvs_values(kvs):
-    """Test KVS values()"""
+def test_kvs_values(kvs: KeyValueStore):
+    """Test KVS values().
+
+    :param kvs: A key-value store populated with data
+    """
     # We get back a KVSValuesView which is just a collections.abc.ValuesView in
     # disguise.
     values = kvs.values()
@@ -87,8 +104,11 @@ def test_kvs_values(kvs):
     assert sorted(list(values)) == ["green", "red", "red", "yellow"]
 
 
-def test_kvs_items(kvs):
-    """Test KVS items()"""
+def test_kvs_items(kvs: KeyValueStore):
+    """Test KVS items().
+
+    :param kvs: A key-value store populated with data
+    """
     # We get back a KVSItemsView which is just a collections.abc.ItemsView in
     # disguise.
     items = kvs.items()
@@ -103,8 +123,12 @@ def test_kvs_items(kvs):
     ]
 
 
-def test_kvs_repr(kvs, empty_kvs):
-    """Test KVS __repr__()"""
+def test_kvs_repr(kvs: KeyValueStore, empty_kvs: KeyValueStore):
+    """Test KVS __repr__().
+
+    :param empty_kvs: An empty key-value store
+    :param kvs: A key-value store populated with data
+    """
     assert repr(kvs) == (
         "KeyValueStore([('apple', 'red'), ('banana', 'yellow'), "
         "('grape', 'green'), ('strawberry', 'red')])"