Repository: SweBarre/sps

Issue:
change cache functionallity
Change the way the cache file is working to be able to handle other types of cached data
example is making it possible to cache information for patch search #14 

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 72bd41d..5b0d703 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@ All notable changes will be documented in this file.
 - --version/-v option
 
 ### Changed
+- changed default cache file name to ~/.cache/sps_cache.json
 - Product/Package headings in README.rst
 - description of sps in README.rst
 - typo i README.rst
diff --git a/Makefile b/Makefile
index 8d08fb3..a5128d6 100644
--- a/Makefile
+++ b/Makefile
@@ -27,7 +27,7 @@ black-diff: guard-PIPENV_ACTIVE
 	black --diff src/ tests/
 
 safety: guard-PIPENV_ACTIVE
-	pip freeze | safety check --stdin
+	pipenv lock --requirements | safety check --stdin
 
 tag: guard-PIPENV_ACTIVE wheel
 	@bash tag.sh
diff --git a/README.rst b/README.rst
index f2c0570..2f6d412 100644
--- a/README.rst
+++ b/README.rst
@@ -30,7 +30,7 @@ Usage
       -h, --help            show this help message and exit
       --cache-file CACHE_FILE, -C CACHE_FILE
                             cache file to use, (default:
-                            /home/jonas/.cache/sps_products.json
+                            /home/jonas/.cache/sps_cache.json
       --version, -v         show program's version number and exit
 
 Shell completion
@@ -64,7 +64,7 @@ Product search
       -h, --help            show this help message and exit
       --cache-file CACHE_FILE, -C CACHE_FILE
                             cache file to use, (default:
-                            /home/jonas/.cache/sps_products.json
+                            /home/jonas/.cache/sps_cache.json
       --version, -v         show program's version number and exit
       --update-cache, -u    Update the local product cache
       --no-cache, -N        Don't use the local cache
@@ -115,7 +115,7 @@ Package search
       -h, --help            show this help message and exit
       --cache-file CACHE_FILE, -C CACHE_FILE
                             cache file to use, (default:
-                            /home/jonas/.cache/sps_products.json
+                            /home/jonas/.cache/sps_cache.json
       --version, -v         show program's version number and exit
       --exact-match, -e     Only show where PATTERN matches exact
       --sort-table {Name,Version,Release,Arch,Module}, -S {Name,Version,Release,Arch,Module}
diff --git a/src/sps/cache.py b/src/sps/cache.py
index 9a9067b..0b940dd 100644
--- a/src/sps/cache.py
+++ b/src/sps/cache.py
@@ -1,22 +1,77 @@
 import json
 import sys
+from pathlib import Path
 
 
-def save(filename, data):
-    if not isinstance(data, dict):
-        print(
-            f"Error: Must have dictionary as data source to save cache", file=sys.stderr
-        )
+def save(key, filename, data):
+    """Saves cache to filename
+
+    The cache is a json file and is loaded as dictionary.
+
+    Parameters
+    ---------
+    key: str
+        the dictionary key that the data should be accociated with.
+    filename: str
+        the path to filename to save the cache to
+    data: list
+        data to be saved in cache
+
+    Returns
+    -------
+    None
+
+    Raises
+    ------
+    SystemExit
+        if key is not known
+        if data is not of type list
+        if PermissionError is raised when saving to filename
+    """
+
+    cache_data = {}
+    if not key in ["product"]:
+        print(f"Error: trying to save cache with unknown key '{key}'", file=sys.stderr)
         sys.exit(1)
+    if not isinstance(data, list):
+        print(f"Error: Must have list as data source to save cache", file=sys.stderr)
+        sys.exit(1)
+
+    if Path(filename).is_file():
+        cache_data = load(filename)
+
+    cache_data[key] = data
+
     try:
         with open(filename, "w") as f:
-            json.dump(data, f)
+            json.dump(cache_data, f)
     except PermissionError as err:
         print(f"Error: {err}", file=sys.stderr)
         sys.exit(13)
 
 
 def load(filename):
+    """Load cache from filename
+
+    The cache file is a json file and retruns a dictionary
+
+    Parameters
+    ----------
+    filename: str
+        path to the json file
+
+    Returns
+    -------
+    dict
+        the content of the cache json file as dict
+
+    Raises
+    ------
+    SystemExit
+        If PermissionError raises
+        if unable to decode cache file as JSON
+    """
+
     try:
         with open(filename, "r") as f:
             data = json.load(f)
@@ -29,9 +84,31 @@ def load(filename):
     return data
 
 
-def lookup(identifier, filename):
+def lookup_product(identifier, filename):
+    """Lookup a product in the cache
+
+    Lookup a product in cache based on identifier and return the id
+
+    Parameters
+    ----------
+    identifier: str
+        product identifier to search for
+    filename: str
+        path to cache file to use
+
+    Returns
+    -------
+    int
+        the product id
+
+    Raises
+    ------
+    SystemExit
+        if no product matches the identifier
+    """
+
     data = load(filename)
-    for product in data["data"]:
+    for product in data["product"]:
         if product["identifier"] == identifier:
             return product["id"]
     print(f"Couldn't find id for '{identifier}'", file=sys.stderr)
diff --git a/src/sps/cli.py b/src/sps/cli.py
index 8d486d6..ebbb513 100644
--- a/src/sps/cli.py
+++ b/src/sps/cli.py
@@ -6,6 +6,21 @@ from sps import products, packages, completion, __version__
 
 
 def create_parser(args=sys.argv[1:]):
+    """Create a argparse.ArgumentParser
+
+    Creates a ArgumentParser based on command line
+
+    Parameters
+    ----------
+    args: list, optional
+        a list that represents how it was called from the command line
+        if not specified the list is build wid sys.argv[1:]
+
+    Returns
+    -------
+    argparse.ArgumentParser
+    """
+
     parser = ArgumentParser()
     for opt in ["-C", "--cache-file"]:
         try:
@@ -18,7 +33,7 @@ def create_parser(args=sys.argv[1:]):
     parser.add_argument(
         "--cache-file",
         "-C",
-        help=f"cache file to use, (default: { str(Path.home()) }/.cache/sps_products.json",
+        help=f"cache file to use, (default: { str(Path.home()) }/.cache/sps_cache.json",
         default=f"{ str(Path.home()) }/.cache/sps_products.json",
     )
     parser.add_argument(
@@ -100,19 +115,21 @@ def create_parser(args=sys.argv[1:]):
 
 
 def main():
+    """The main program logic"""
+
     parser = create_parser()
     args = parser.parse_args()
 
     if args.command in ["product", "package"]:
         table = PrettyTable()
         if args.command == "product":
-            product_data = products.get(
+            products_data = products.get(
                 args.pattern, args.cache_file, args.no_cache, args.update_cache
             )
             table.field_names = ["id", "Name", "Edition", "Identifier", "Arch"]
             for name in table.field_names:
                 table.align[name] = "l"
-            for product in product_data["data"]:
+            for product in products_data:
                 table.add_row(
                     [
                         product["id"],
@@ -133,7 +150,7 @@ def main():
             table.field_names = ["Name", "Version", "Release", "Arch", "Module"]
             for name in table.field_names:
                 table.align[name] = "l"
-            for package in package_data["data"]:
+            for package in package_data:
                 module_line = ""
                 for product in package["products"]:
                     module_line = "{},{}".format(module_line, product["name"])
diff --git a/src/sps/completion.py b/src/sps/completion.py
index 6fc8743..fa6e933 100644
--- a/src/sps/completion.py
+++ b/src/sps/completion.py
@@ -3,6 +3,25 @@ import os
 
 
 def get(cachefile, shell=None):
+    """Generates a shell completion script
+
+    Parameters
+    ----------
+    cachefile: str
+        Path to cache file
+    shell: str, optional
+        Name of the shell to generate completion script for
+        if not provided the function will try to use the 
+        SHELL environment varable
+
+    Raises
+    ------
+    SystemExit
+        if an unknown shell is provided
+        if shell is not provided and SHELL environ is not set
+        if completion generation file is not readable
+    """
+
     if shell == None:
         try:
             os.environ["SHELL"]
diff --git a/src/sps/packages.py b/src/sps/packages.py
index 1cf4a6f..a460044 100644
--- a/src/sps/packages.py
+++ b/src/sps/packages.py
@@ -3,10 +3,36 @@ from sps import request, cache
 
 
 def get(product, pattern, cache_filename):
+    """Get the packages from SUSE Customer Center 
+    for a particular product
+
+    Parameters
+    ----------
+    product: str
+        product id or identifier
+    pattern: str
+        search pattern in package name, if blank all packages will be
+        returned
+    cache_filename: str
+        path to the cache file name to be used to lookup product id
+        if product identifier is specified instead of product it
+
+    Returns
+    -------
+    list
+        a list of packages that matches the search result
+        each package is a dict
+
+    Raises
+    ------
+    SystemExit
+        if the response from SUSE Customer Center is not parsable
+    """
+
     try:
         product_id = int(product)
     except ValueError:
-        product_id = cache.lookup(product, cache_filename)
+        product_id = cache.lookup_product(product, cache_filename)
 
     url = f"https://scc.suse.com/api/package_search/packages?product_id={product_id}&query={pattern}"
     response = request.fetch(url)
@@ -15,4 +41,4 @@ def get(product, pattern, cache_filename):
     except KeyError as err:
         print(f"Error: No data key found, {err}", file=sys.stderr)
         sys.exit(1)
-    return response
+    return response["data"]
diff --git a/src/sps/products.py b/src/sps/products.py
index 9d99b93..ec46afe 100644
--- a/src/sps/products.py
+++ b/src/sps/products.py
@@ -5,26 +5,34 @@ from sps import request, cache
 
 def get(pattern, cache_filename, no_cache, update_cache):
     cache_path = Path(cache_filename)
+    data = {}
     if cache_path.exists() and cache_path.is_file() and not no_cache:
-        response = cache.load(cache_filename)
+        data = cache.load(cache_filename)
+        try:
+            data["product"]
+        except KeyError as err:
+            print(f"Error: no product key found in cache file", file=sys.stderr)
+            sys.exit(1)
     else:
         response = request.fetch("https://scc.suse.com/api/package_search/products")
-    try:
-        response["data"]
-    except KeyError as err:
-        print(f"Error: No data key found, {err}", file=sys.stderr)
-        sys.exit(1)
+        try:
+            data["product"] = response["data"]
+        except KeyError as err:
+            print(
+                f"Error: No data key found in scc api response, {err}", file=sys.stderr
+            )
+            sys.exit(1)
 
-    if update_cache:
-        cache.save(cache_filename, response)
-    ret = {"data": []}
+    ret = []
     if not pattern:
-        ret = response
+        ret = data["product"]
     else:
-        for product in response["data"]:
+        for product in data["product"]:
             for k in product.keys():
                 if pattern in str(product[k]):
-                    ret["data"].append(product)
+                    ret.append(product)
                     break
 
+    if update_cache:
+        cache.save("product", cache_filename, ret)
     return ret


Test patch:
diff --git a/tests/test_cache.py b/tests/test_cache.py
index 35be4b8..300b4c9 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -9,7 +9,7 @@ from sps import cache
 @pytest.fixture
 def data():
     return {
-        "data": [
+        "product": [
             {
                 "id": 1899,
                 "name": "SUSE Manager Server",
@@ -37,21 +37,27 @@ def test_cache_save_load(data):
     saves and loads content from a file-like source
     """
     fn = tempfile.NamedTemporaryFile(delete=False)
-    assert cache.save(fn.name, data) == None
+    os.remove(fn.name)
+    assert cache.save("product", fn.name, data["product"]) == None
     assert cache.load(fn.name) == data
     os.remove(fn.name)
 
 
-def test_cache_save_bougus_data():
+def test_cache_save_bogus_data():
     """
     save will exit if bogus data provided
     """
     fn = tempfile.NamedTemporaryFile(delete=False)
     with pytest.raises(SystemExit):
-        cache.save(fn.name, "{age:100}")
+        cache.save("product", fn.name, "{age:100}")
     os.remove(fn.name)
 
 
+def test_cache_save_bogus_key():
+    with pytest.raises(SystemExit):
+        cache.save("fake-cache-key", "fake-file-name", [])
+
+
 def test_cache_load_bougus_data():
     """
     load will exit if bogus data provided
@@ -71,38 +77,57 @@ def test_cache_save_permission_denied(data):
     fn = tempfile.NamedTemporaryFile(delete=False)
     os.chmod(fn.name, stat.S_IRUSR)
     with pytest.raises(SystemExit):
-        cache.save(fn.name, data)
+        cache.save("product", fn.name, data)
     os.remove(fn.name)
 
 
+def test_cache_save_key_load_old(data):
+    fn = tempfile.NamedTemporaryFile(delete=False)
+    existing_data = {"testing": "testing", "product": "product"}
+    with open(fn.name, "w") as f:
+        json.dump(existing_data, f)
+
+    cache.save("product", fn.name, data["product"])
+
+    with open(fn.name, "r") as f:
+        cachedata = json.load(f)
+
+    new_data = {"testing": "testing", "product": data["product"]}
+
+    assert new_data == cachedata
+
+
 def test_cache_load_permission_denied(data):
     """
     load should exit the cache
     """
     fn = tempfile.NamedTemporaryFile(delete=False)
-    cache.save(fn.name, data)
+    os.remove(fn.name)
+    cache.save("product", fn.name, data["product"])
     os.chmod(fn.name, stat.S_IWUSR)
     with pytest.raises(SystemExit):
         cache.load(fn.name)
     os.remove(fn.name)
 
 
-def test_cache_lookup_found(data):
+def test_cache_lookup_product_found(data):
     """
     lookup will not exit if identifier is found
     """
     fn = tempfile.NamedTemporaryFile(delete=False)
-    cache.save(fn.name, data) == None
-    assert cache.lookup("SLED/15.2/x86_64", fn.name) == 1935
+    os.remove(fn.name)
+    cache.save("product", fn.name, data["product"]) == None
+    assert cache.lookup_product("SLED/15.2/x86_64", fn.name) == 1935
     os.remove(fn.name)
 
 
-def test_cache_lookup_notfound(data):
+def test_cache_lookup_product_notfound(data):
     """
     lookup will exit if identifier is not found
     """
     fn = tempfile.NamedTemporaryFile(delete=False)
-    cache.save(fn.name, data) == None
+    os.remove(fn.name)
+    cache.save("product", fn.name, data["product"]) == None
     with pytest.raises(SystemExit):
-        cache.lookup("Bogus product name", fn.name)
+        cache.lookup_product("Bogus product name", fn.name)
     os.remove(fn.name)
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 0d0711d..6395f58 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -277,7 +277,7 @@ def test_main_product_output(mocker, capsys):
     }
 
     mocker.patch("sps.products.get", autospec=True)
-    products.get.return_value = data
+    products.get.return_value = data["data"]
     mocker.patch("sps.cli.create_parser", autospec=True)
     cli.create_parser.return_value = parser_proxy()
 
@@ -323,7 +323,7 @@ def test_main_product_output_short(mocker, capsys):
     }
 
     mocker.patch("sps.products.get", autospec=True)
-    products.get.return_value = data
+    products.get.return_value = data["data"]
     mocker.patch("sps.cli.create_parser", autospec=True)
     cli.create_parser.return_value = parser_proxy()
 
@@ -391,7 +391,7 @@ def test_main_package_output(mocker, capsys):
     }
 
     mocker.patch("sps.packages.get", autospec=True)
-    packages.get.return_value = data
+    packages.get.return_value = data["data"]
     mocker.patch("sps.cli.create_parser", autospec=True)
     cli.create_parser.return_value = parser_proxy()
 
@@ -465,7 +465,7 @@ def test_main_package_output_exact_match(mocker, capsys):
     }
 
     mocker.patch("sps.packages.get", autospec=True)
-    packages.get.return_value = data
+    packages.get.return_value = data["data"]
     mocker.patch("sps.cli.create_parser", autospec=True)
     cli.create_parser.return_value = parser_proxy()
 
diff --git a/tests/test_packages.py b/tests/test_packages.py
index 5b43ab1..b4a948d 100644
--- a/tests/test_packages.py
+++ b/tests/test_packages.py
@@ -31,7 +31,7 @@ def test_product_get_with_id(mocker, requests_response):
     """
     mocker.patch("requests.get", autospec=True)
     requests.get.return_value = requests_response
-    assert packages.get(1899, "vim", "filename") == {"data": [{"foo": "bar"}]}
+    assert packages.get(1899, "vim", "filename") == [{"foo": "bar"}]
     requests.get.assert_called_with(
         "https://scc.suse.com/api/package_search/packages?product_id=1899&query=vim",
         request.headers,
@@ -44,8 +44,8 @@ def test_product_get_with_product_identifier(mocker, requests_response):
     """
     mocker.patch("requests.get", autospec=True)
     requests.get.return_value = requests_response
-    mocker.patch("sps.cache.lookup", autospec=True)
-    cache.lookup.return_value = 1899
+    mocker.patch("sps.cache.lookup_product", autospec=True)
+    cache.lookup_product.return_value = 1899
     packages.get("SLED/15.2/x86_64", "vim", "filename")
     requests.get.assert_called_with(
         "https://scc.suse.com/api/package_search/packages?product_id=1899&query=vim",
diff --git a/tests/test_products.py b/tests/test_products.py
index d9a7413..1a6ebb3 100644
--- a/tests/test_products.py
+++ b/tests/test_products.py
@@ -44,9 +44,9 @@ def test_products_get_pattern_multiple_match(data, mocker):
     """
     mocker.patch("sps.request.fetch", autospec=True)
     request.fetch.return_value = data
-    assert products.get("1", "fake-file-name", False, False) == data
-    assert products.get("SUSE", "fake-file-name", False, False) == data
-    assert products.get("x86", "fake-file-name", False, False) == data
+    assert products.get("1", "fake-file-name", False, False) == data["data"]
+    assert products.get("SUSE", "fake-file-name", False, False) == data["data"]
+    assert products.get("x86", "fake-file-name", False, False) == data["data"]
 
 
 def test_products_get_pattern_single_match(data, mocker):
@@ -55,12 +55,8 @@ def test_products_get_pattern_single_match(data, mocker):
     """
     mocker.patch("sps.request.fetch", autospec=True)
     request.fetch.return_value = data
-    assert products.get("Manager", "fake-file-name", False, False) == {
-        "data": [data["data"][0]]
-    }
-    assert products.get("Desktop", "fake-file-name", False, False) == {
-        "data": [data["data"][1]]
-    }
+    assert products.get("Manager", "fake-file-name", False, False) == [data["data"][0]]
+    assert products.get("Desktop", "fake-file-name", False, False) == [data["data"][1]]
 
 
 def test_products_get_pattern_no_match(data, mocker):
@@ -69,9 +65,7 @@ def test_products_get_pattern_no_match(data, mocker):
     """
     mocker.patch("sps.request.fetch", autospec=True)
     request.fetch.return_value = data
-    assert products.get("foo-bar-search", "fake-file-name", False, False) == {
-        "data": []
-    }
+    assert products.get("foo-bar-search", "fake-file-name", False, False) == []
 
 
 def test_products_get(data, mocker):
@@ -90,10 +84,8 @@ def test_products_get_wrong_cache_data(data, mocker):
     """
     get will exit if no data field is found in response from cache
     """
-    wrongdata = {}
-    wrongdata["bad"] = data["data"]
     mocker.patch("sps.cache.load", autospec=True)
-    cache.load.return_value = wrongdata
+    cache.load.return_value = data
     with pytest.raises(SystemExit):
         products.get(None, __file__, False, False)
 
@@ -103,8 +95,8 @@ def test_products_get_cache(data, mocker):
     get wtil retrieve products from cache file
     """
     mocker.patch("sps.cache.load", autospec=True)
-    cache.load.return_value = data
-    assert products.get(None, __file__, False, False) == data
+    cache.load.return_value = {"product": data["data"]}
+    assert products.get(None, __file__, False, False) == data["data"]
 
 
 def test_products_update_cache(data, mocker):
@@ -115,4 +107,4 @@ def test_products_update_cache(data, mocker):
     mocker.patch("sps.request.fetch", autospec=True)
     request.fetch.return_value = data
     products.get(None, "fake-file-name", False, True)
-    cache.save.assert_called_with("fake-file-name", data)
+    cache.save.assert_called_with("product", "fake-file-name", data["data"])