Add clarification that settings must be explicitly set to save

Add unit test, that is clear that docstrings should be updated
if the behaviour changes

Author: julianstirling

src/labthings_fastapi/decorators/__init__.py

@@ -111,6 +111,12 @@ def thing_setting(func: Callable) -> ThingSetting:
     If the type is a pydantic BaseModel, then the setter must also be able to accept
     the dictionary representation of this BaseModel as this is what will be used to
     set the Setting when loading from disk on starting the server.
+
+    Note: If a setting updated, rather than explicitly set this will not trigger saving.
+    For example: if a Thing has a setting called `dictsetting` holding the dictionary
+    `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving
+    but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never
+    called.
     """
     # Replace the function with a `Descriptor` that's a `ThingSetting`
     return ThingSetting(

src/labthings_fastapi/descriptors/property.py

@@ -224,7 +224,7 @@ def getter(self, func: Callable) -> Self:
     def setter(self, func: Callable) -> Self:
         """Decorator to set the property's value
 
-        ThingPropertys are variabes - so they will return the value they hold
+        ThingPropertys are variables - so they will return the value they hold
         when they are accessed. However, they can run code when they are set: this
         decorator sets a function as that code.
         """
@@ -239,6 +239,12 @@ class ThingSetting(ThingProperty):
     A ThingSetting is a ThingProperty with extra functionality for triggering
     a Thing to save its settings.
 
+    Note: If a setting updated, rather than explicitly set this will not trigger saving.
+    For example: if a Thing has a setting called `dictsetting` holding the dictionary
+    `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving
+    but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never
+    called.
+
     The setting otherwise acts just like a normal variable.
     """
 

tests/test_settings.py

@@ -16,6 +16,9 @@
 class TestThing(Thing):
     boolsetting = ThingSetting(bool, False, description="A boolean setting")
     stringsetting = ThingSetting(str, "foo", description="A string setting")
+    dictsetting = ThingSetting(
+        dict, {"a": 1, "b": 2}, description="A dictionary setting"
+    )
 
     _float = 1.0
 
@@ -42,15 +45,20 @@ def _get_setting_file(server, thingpath):
     return os.path.normpath(path)
 
 
-def _settings_dict(boolsetting=False, floatsetting=1.0, stringsetting="foo"):
+def _settings_dict(
+    boolsetting=False, floatsetting=1.0, stringsetting="foo", dictsetting=None
+):
     """Return the expected settings dictionary
 
     Parameters can be updated from default values
     """
+    if dictsetting is None:
+        dictsetting = {"a": 1, "b": 2}
     return {
         "boolsetting": boolsetting,
         "floatsetting": floatsetting,
         "stringsetting": stringsetting,
+        "dictsetting": dictsetting,
     }
 
 
@@ -91,6 +99,37 @@ def test_settings_save(thing, server):
             assert json.load(file_obj) == _settings_dict(floatsetting=2.0)
 
 
+def test_settings_dict_save(thing, server):
+    """Check settings are saved if the dict is updated in full"""
+    setting_file = _get_setting_file(server, "/thing")
+    server.add_thing(thing, "/thing")
+    # No setting file created when first added
+    assert not os.path.isfile(setting_file)
+    with TestClient(server.app):
+        thing.dictsetting = {"c": 3}
+        assert os.path.isfile(setting_file)
+        with open(setting_file, "r", encoding="utf-8") as file_obj:
+            # Check settings on file match expected dictionary
+            assert json.load(file_obj) == _settings_dict(dictsetting={"c": 3})
+
+
+def test_settings_dict_internal_update(thing, server):
+    """Confirm settings are not saved if the internal value of a dictionary is updated
+
+    This behaviour is not ideal, but it is documented. If the behaviour is updated
+    then the documentation should be updated and this test removed
+    """
+    setting_file = _get_setting_file(server, "/thing")
+    server.add_thing(thing, "/thing")
+    # No setting file created when first added
+    assert not os.path.isfile(setting_file)
+    with TestClient(server.app):
+        thing.dictsetting["a"] = 4
+        # As only an internal member of the dictornary was set, the saving was not
+        # triggered.
+        assert not os.path.isfile(setting_file)
+
+
 def test_settings_load(thing, server):
     """Check settings can be loaded from disk when added to server"""
     setting_file = _get_setting_file(server, "/thing")