Fixes typo in Caching envvar (#424) (#426)

* Fixes typo in Caching envvar Envvar to set caching mode was previously CLOUPATHLIB_FILE_CACHE_MODE (missing D in CLOUDPATHLIB). This commit fixes this typo and introduces some code to help migrate projects to the new envvar. The current logic for migration warnings is: - If the old envvar is set and not the new one, use the old one and issue a deprecation warning - If the old and new envvars are set, use the new version and issue a warning if the two disagree * Added deprecation version * Fully reset envvars after testing Co-authored-by: Micha Gorelick <[email protected]>
drivendataorg · Apr 9, 2024 · fabb0e4 · fabb0e4
1 parent d368501
commit fabb0e4
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 13 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -2,6 +2,7 @@
 
 ## UNRELEASED
 
+- Fixes typo in `FileCacheMode` where values were being filled by envvar `CLOUPATHLIB_FILE_CACHE_MODE` instead of `CLOUDPATHLIB_FILE_CACHE_MODE`. (PR [#424](https://github.com/drivendataorg/cloudpathlib/pull/424)
 - Fix `CloudPath` cleanup via `CloudPath.__del__` when `Client` encounters an exception during initialization and does not create a `file_cache_mode` attribute. (Issue [#372](https://github.com/drivendataorg/cloudpathlib/issues/372), thanks to [@bryanwweber](https://github.com/bryanwweber)) 
 - Drop support for Python 3.7; pin minimal `boto3` version to Python 3.8+ versions. (PR [#407](https://github.com/drivendataorg/cloudpathlib/pull/407))
 - fix: use native `exists()` method in `GSClient`. (PR [#420](https://github.com/drivendataorg/cloudpathlib/pull/420))

diff --git a/cloudpathlib/enums.py b/cloudpathlib/enums.py
@@ -1,4 +1,5 @@
 from enum import Enum
+import warnings
 import os
 from typing import Optional
 
@@ -16,7 +17,7 @@ class FileCacheMode(str, Enum):
         close_file (str): Cache for a `CloudPath` file is removed as soon as the file is closed. Note: you must
             use `CloudPath.open` whenever opening the file for this method to function.
 
-    Modes can be set by passing them to the Client or by setting the `CLOUPATHLIB_FILE_CACHE_MODE`
+    Modes can be set by passing them to the Client or by setting the `CLOUDPATHLIB_FILE_CACHE_MODE`
     environment variable.
 
     For more detail, see the [caching documentation page](../../caching).
@@ -29,14 +30,33 @@ class FileCacheMode(str, Enum):
 
     @classmethod
     def from_environment(cls) -> Optional["FileCacheMode"]:
-        """Parses the environment variable `CLOUPATHLIB_FILE_CACHE_MODE` into
+        """Parses the environment variable `CLOUDPATHLIB_FILE_CACHE_MODE` into
         an instance of this Enum.
 
         Returns:
             FileCacheMode enum value if the env var is defined, else None.
         """
-        env_string = os.environ.get("CLOUPATHLIB_FILE_CACHE_MODE", "").lower()
 
+        env_string = os.environ.get("CLOUDPATHLIB_FILE_CACHE_MODE", "").lower()
+        env_string_typo = os.environ.get("CLOUPATHLIB_FILE_CACHE_MODE", "").lower()
+
+        if env_string_typo:
+            warnings.warn(
+                "envvar CLOUPATHLIB_FILE_CACHE_MODE has been renamed to "
+                "CLOUDPATHLIB_FILE_CACHE_MODE. Reading from the old value "
+                "will become deprecated in version 0.20.0",
+                DeprecationWarning,
+            )
+
+        if env_string and env_string_typo and env_string != env_string_typo:
+            warnings.warn(
+                "CLOUDPATHLIB_FILE_CACHE_MODE and CLOUPATHLIB_FILE_CACHE_MODE "
+                "envvars set to different values. Disregarding old value and "
+                f"using CLOUDPATHLIB_FILE_CACHE_MODE = {env_string}",
+                RuntimeWarning,
+            )
+
+        env_string = env_string or env_string_typo
         if not env_string:
             return None
         else:

diff --git a/docs/docs/caching.ipynb b/docs/docs/caching.ipynb
@@ -460,9 +460,9 @@
     "\n",
     "### Setting the cache clearing method\n",
     "\n",
-    "You can set the cache clearing method either through the environment variable `CLOUPATHLIB_FILE_CACHE_MODE` or by passing the mode to the `*Client` when you instantiate it. See below for an example.\n",
+    "You can set the cache clearing method either through the environment variable `CLOUDPATHLIB_FILE_CACHE_MODE` or by passing the mode to the `*Client` when you instantiate it. See below for an example.\n",
     "\n",
-    "You can set `CLOUPATHLIB_FILE_CACHE_MODE` to any of the supported values, which are printed below."
+    "You can set `CLOUDPATHLIB_FILE_CACHE_MODE` to any of the supported values, which are printed below."
    ]
   },
   {
@@ -727,7 +727,7 @@
      "traceback": [
       "\u001b[0;31m---------------------------------------------------------------------------\u001b[0m",
       "\u001b[0;31mInvalidConfigurationException\u001b[0m             Traceback (most recent call last)",
-      "Cell \u001b[0;32mIn[18], line 6\u001b[0m\n\u001b[1;32m      3\u001b[0m \u001b[38;5;66;03m# set the mode here so that it will be used when we instantiate the client\u001b[39;00m\n\u001b[1;32m      4\u001b[0m os\u001b[38;5;241m.\u001b[39menviron[\u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mCLOUPATHLIB_FILE_CACHE_MODE\u001b[39m\u001b[38;5;124m\"\u001b[39m] \u001b[38;5;241m=\u001b[39m \u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mpersistent\u001b[39m\u001b[38;5;124m\"\u001b[39m\n\u001b[0;32m----> 6\u001b[0m tmp_dir_client \u001b[38;5;241m=\u001b[39m \u001b[43mS3Client\u001b[49m\u001b[43m(\u001b[49m\u001b[43m)\u001b[49m\n",
+      "Cell \u001b[0;32mIn[18], line 6\u001b[0m\n\u001b[1;32m      3\u001b[0m \u001b[38;5;66;03m# set the mode here so that it will be used when we instantiate the client\u001b[39;00m\n\u001b[1;32m      4\u001b[0m os\u001b[38;5;241m.\u001b[39menviron[\u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mCLOUDPATHLIB_FILE_CACHE_MODE\u001b[39m\u001b[38;5;124m\"\u001b[39m] \u001b[38;5;241m=\u001b[39m \u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mpersistent\u001b[39m\u001b[38;5;124m\"\u001b[39m\n\u001b[0;32m----> 6\u001b[0m tmp_dir_client \u001b[38;5;241m=\u001b[39m \u001b[43mS3Client\u001b[49m\u001b[43m(\u001b[49m\u001b[43m)\u001b[49m\n",
       "File \u001b[0;32m~/code/cloudpathlib/cloudpathlib/s3/s3client.py:129\u001b[0m, in \u001b[0;36mS3Client.__init__\u001b[0;34m(self, aws_access_key_id, aws_secret_access_key, aws_session_token, no_sign_request, botocore_session, profile_name, boto3_session, file_cache_mode, local_cache_dir, endpoint_url, boto3_transfer_config, content_type_method, extra_args)\u001b[0m\n\u001b[1;32m    121\u001b[0m \u001b[38;5;66;03m# listing ops (list_objects_v2, filter, delete) only accept these extras:\u001b[39;00m\n\u001b[1;32m    122\u001b[0m \u001b[38;5;66;03m# https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html\u001b[39;00m\n\u001b[1;32m    123\u001b[0m \u001b[38;5;28mself\u001b[39m\u001b[38;5;241m.\u001b[39mboto3_list_extra_args \u001b[38;5;241m=\u001b[39m {\n\u001b[1;32m    124\u001b[0m     k: \u001b[38;5;28mself\u001b[39m\u001b[38;5;241m.\u001b[39m_extra_args[k]\n\u001b[1;32m    125\u001b[0m     \u001b[38;5;28;01mfor\u001b[39;00m k \u001b[38;5;129;01min\u001b[39;00m [\u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mRequestPayer\u001b[39m\u001b[38;5;124m\"\u001b[39m, \u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mExpectedBucketOwner\u001b[39m\u001b[38;5;124m\"\u001b[39m]\n\u001b[1;32m    126\u001b[0m     \u001b[38;5;28;01mif\u001b[39;00m k \u001b[38;5;129;01min\u001b[39;00m \u001b[38;5;28mself\u001b[39m\u001b[38;5;241m.\u001b[39m_extra_args\n\u001b[1;32m    127\u001b[0m }\n\u001b[0;32m--> 129\u001b[0m \u001b[38;5;28;43msuper\u001b[39;49m\u001b[43m(\u001b[49m\u001b[43m)\u001b[49m\u001b[38;5;241;43m.\u001b[39;49m\u001b[38;5;21;43m__init__\u001b[39;49m\u001b[43m(\u001b[49m\n\u001b[1;32m    130\u001b[0m \u001b[43m    \u001b[49m\u001b[43mlocal_cache_dir\u001b[49m\u001b[38;5;241;43m=\u001b[39;49m\u001b[43mlocal_cache_dir\u001b[49m\u001b[43m,\u001b[49m\n\u001b[1;32m    131\u001b[0m \u001b[43m    \u001b[49m\u001b[43mcontent_type_method\u001b[49m\u001b[38;5;241;43m=\u001b[39;49m\u001b[43mcontent_type_method\u001b[49m\u001b[43m,\u001b[49m\n\u001b[1;32m    132\u001b[0m \u001b[43m    \u001b[49m\u001b[43mfile_cache_mode\u001b[49m\u001b[38;5;241;43m=\u001b[39;49m\u001b[43mfile_cache_mode\u001b[49m\u001b[43m,\u001b[49m\n\u001b[1;32m    133\u001b[0m \u001b[43m\u001b[49m\u001b[43m)\u001b[49m\n",
       "File \u001b[0;32m~/code/cloudpathlib/cloudpathlib/client.py:57\u001b[0m, in \u001b[0;36mClient.__init__\u001b[0;34m(self, file_cache_mode, local_cache_dir, content_type_method)\u001b[0m\n\u001b[1;32m     54\u001b[0m     file_cache_mode \u001b[38;5;241m=\u001b[39m FileCacheMode\u001b[38;5;241m.\u001b[39mpersistent\n\u001b[1;32m     56\u001b[0m \u001b[38;5;28;01mif\u001b[39;00m file_cache_mode \u001b[38;5;241m==\u001b[39m FileCacheMode\u001b[38;5;241m.\u001b[39mpersistent \u001b[38;5;129;01mand\u001b[39;00m local_cache_dir \u001b[38;5;129;01mis\u001b[39;00m \u001b[38;5;28;01mNone\u001b[39;00m:\n\u001b[0;32m---> 57\u001b[0m     \u001b[38;5;28;01mraise\u001b[39;00m InvalidConfigurationException(\n\u001b[1;32m     58\u001b[0m         \u001b[38;5;124mf\u001b[39m\u001b[38;5;124m\"\u001b[39m\u001b[38;5;124mIf you use the \u001b[39m\u001b[38;5;124m'\u001b[39m\u001b[38;5;132;01m{\u001b[39;00mFileCacheMode\u001b[38;5;241m.\u001b[39mpersistent\u001b[38;5;132;01m}\u001b[39;00m\u001b[38;5;124m'\u001b[39m\u001b[38;5;124m cache mode, you must pass a `local_cache_dir` when you instantiate the client.\u001b[39m\u001b[38;5;124m\"\u001b[39m\n\u001b[1;32m     59\u001b[0m     )\n\u001b[1;32m     61\u001b[0m \u001b[38;5;66;03m# if no explicit local dir, setup caching in temporary dir\u001b[39;00m\n\u001b[1;32m     62\u001b[0m \u001b[38;5;28;01mif\u001b[39;00m local_cache_dir \u001b[38;5;129;01mis\u001b[39;00m \u001b[38;5;28;01mNone\u001b[39;00m:\n",
       "\u001b[0;31mInvalidConfigurationException\u001b[0m: If you use the 'FileCacheMode.persistent' cache mode, you must pass a `local_cache_dir` when you instantiate the client."
@@ -738,7 +738,7 @@
     "import os\n",
     "\n",
     "# set the mode here so that it will be used when we instantiate the client\n",
-    "os.environ[\"CLOUPATHLIB_FILE_CACHE_MODE\"] = \"persistent\"\n",
+    "os.environ[\"CLOUDPATHLIB_FILE_CACHE_MODE\"] = \"persistent\"\n",
     "\n",
     "tmp_dir_client = S3Client()"
    ]

diff --git a/docs/docs/script/caching.py b/docs/docs/script/caching.py
@@ -182,9 +182,9 @@
 # 
 # ### Setting the cache clearing method
 # 
-# You can set the cache clearing method either through the environment variable `CLOUPATHLIB_FILE_CACHE_MODE` or by passing the mode to the `*Client` when you instantiate it. See below for an example.
+# You can set the cache clearing method either through the environment variable `CLOUDPATHLIB_FILE_CACHE_MODE` or by passing the mode to the `*Client` when you instantiate it. See below for an example.
 # 
-# You can set `CLOUPATHLIB_FILE_CACHE_MODE` to any of the supported values, which are printed below.
+# You can set `CLOUDPATHLIB_FILE_CACHE_MODE` to any of the supported values, which are printed below.
 
 from cloudpathlib.enums import FileCacheMode
 
@@ -331,7 +331,7 @@
 import os
 
 # set the mode here so that it will be used when we instantiate the client
-os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = "persistent"
+os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = "persistent"
 
 tmp_dir_client = S3Client()
 

diff --git a/tests/test_caching.py b/tests/test_caching.py
@@ -270,19 +270,56 @@ def test_string_instantiation(rig: CloudProviderTestRig, tmpdir):
         assert client.file_cache_mode == v
 
 
-def test_environment_variable_instantiation(rig: CloudProviderTestRig, tmpdir):
+def test_environment_variable_contentious_instantiation(rig: CloudProviderTestRig, tmpdir):
+    # environment instantiation
+    original_typo_env_setting = os.environ.get("CLOUPATHLIB_FILE_CACHE_MODE", "")
+    original_env_setting = os.environ.get("CLOUDPATHLIB_FILE_CACHE_MODE", "")
+
+    v_old = FileCacheMode.persistent
+    try:
+        for v in FileCacheMode:
+            os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = v_old.value
+            os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = v.value
+            local = tmpdir if v == FileCacheMode.persistent else None
+            client = rig.client_class(local_cache_dir=local, **rig.required_client_kwargs)
+            assert client.file_cache_mode == v
+
+    finally:
+        os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = original_typo_env_setting
+        os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = original_env_setting
+
+
+def test_environment_variable_old_instantiation(rig: CloudProviderTestRig, tmpdir):
     # environment instantiation
-    original_env_setting = os.environ.get("CLOUPATHLIB_FILE_CACHE_MODE", "")
+    original_typo_env_setting = os.environ.get("CLOUPATHLIB_FILE_CACHE_MODE", "")
+    original_env_setting = os.environ.get("CLOUDPATHLIB_FILE_CACHE_MODE", "")
 
     try:
+        os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = ""
         for v in FileCacheMode:
             os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = v.value
             local = tmpdir if v == FileCacheMode.persistent else None
             client = rig.client_class(local_cache_dir=local, **rig.required_client_kwargs)
             assert client.file_cache_mode == v
 
     finally:
-        os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = original_env_setting
+        os.environ["CLOUPATHLIB_FILE_CACHE_MODE"] = original_typo_env_setting
+        os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = original_env_setting
+
+
+def test_environment_variable_instantiation(rig: CloudProviderTestRig, tmpdir):
+    # environment instantiation
+    original_env_setting = os.environ.get("CLOUDPATHLIB_FILE_CACHE_MODE", "")
+
+    try:
+        for v in FileCacheMode:
+            os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = v.value
+            local = tmpdir if v == FileCacheMode.persistent else None
+            client = rig.client_class(local_cache_dir=local, **rig.required_client_kwargs)
+            assert client.file_cache_mode == v
+
+    finally:
+        os.environ["CLOUDPATHLIB_FILE_CACHE_MODE"] = original_env_setting
 
 
 def test_environment_variable_local_cache_dir(rig: CloudProviderTestRig, tmpdir):