Repository: XLSForm/pyxform
Pull request #755 (2025-02-13 07:00:35)

748: handle obj.get(k, default) call pattern for backward compatibility
Closes #748

#### Why is this the best possible solution? Were any other approaches considered?

- Allows external libs to continue using `obj.get(key, default)` with SurveyElement if needed
- Encourages migration to new (or alternative) call patterns with ignorable DeprecationWarning
- Tests demonstrate expected behaviour for range of call patterns, for clarification

Alternatives

The original code in #748 meant that `elem.get("foo")` would return None. This seemed seemed like it would be confusing for pyxform maintenance since a) there is still some `elem.get("foo")` code, and b) its easier to only be concerned about AttributeErrors (as would be expected for a non-dict class) rather than a mix of None, KeyError, AttributeError. If calling code like `elem.get("foo")` expects None then it can be adapted to use an explicit `None` default e.g. `elem.get("foo", None)` - otherwise it will raise an AttributeError as before. The `SurveyElement` also differs from `dict` in `elem["foo"]` (AttributeError not KeyError, respectively) so it didn't seem appropriate to fully replicate `dict` behaviour in this custom Mapping class.

Other alternatives mentioned in #748 and the warning message, for external libs to implement:

- find-and-replace to swap obj.get(key, default) to getattr(obj, key, default)
- checking the type with isinstance before accessing a potentially non-existent attribute
- try/catch for AttributeError if the attribute doesn't exist
- create a custom subclass that implements get in the preferred way

#### What are the regression risks?

There are no internal calls like `obj.get(key, default)`. External code should work as before, except for the DeprecationWarning which can be ignored as shown in the tests, i.e. add:

```
warnings.simplefilter("ignore", DeprecationWarning)
```

#### Does this change require updates to documentation? If so, please file an issue [here](https://github.com/XLSForm/xlsform.github.io) and include the link below.

No.

#### Before submitting this PR, please make sure you have:
- [x] included test cases for core behavior and edge cases in `tests`
- [x] run `python -m unittest` and verified all tests pass
- [x]  run `ruff format pyxform tests` and `ruff check pyxform tests` to lint code
- [x] verified that any code or assets from external sources are properly credited in comments


Patch:
diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py
index b6c851b..3312e22 100644
--- a/pyxform/survey_element.py
+++ b/pyxform/survey_element.py
@@ -4,6 +4,7 @@ Survey Element base class for all survey elements.
 
 import json
 import re
+import warnings
 from collections.abc import Callable, Generator, Iterable, Mapping
 from itertools import chain
 from typing import TYPE_CHECKING, Optional
@@ -35,6 +36,7 @@ SURVEY_ELEMENT_FIELDS = (
 )
 SURVEY_ELEMENT_EXTRA_FIELDS = ("_survey_element_xpath",)
 SURVEY_ELEMENT_SLOTS = (*SURVEY_ELEMENT_FIELDS, *SURVEY_ELEMENT_EXTRA_FIELDS)
+_GET_SENTINEL = object()
 
 
 class SurveyElement(Mapping):
@@ -53,6 +55,22 @@ class SurveyElement(Mapping):
     def __getitem__(self, key):
         return self.__getattribute__(key)
 
+    def get(self, key, default=_GET_SENTINEL):
+        try:
+            return self.__getattribute__(key)
+        except AttributeError:
+            # Sentinel used rather than None since caller may write `default=None`.
+            if default is _GET_SENTINEL:
+                raise
+            warnings.warn(
+                "The `obj.get(key, default)` usage pattern will be removed in a future "
+                "version of pyxform. Please check the object type to ensure the "
+                "attribute will exist, or use `hasattr(obj, key, default)` instead.",
+                DeprecationWarning,
+                stacklevel=2,  # level 1 = here, 2 = caller.
+            )
+            return default
+
     @staticmethod
     def get_slot_names() -> tuple[str, ...]:
         """Each subclass must provide a list of slots from itself and all parents."""


Test patch:
diff --git a/tests/test_survey_element.py b/tests/test_survey_element.py
new file mode 100644
index 0000000..6eb8857
--- /dev/null
+++ b/tests/test_survey_element.py
@@ -0,0 +1,67 @@
+import warnings
+from unittest import TestCase
+
+from pyxform.survey_element import SurveyElement
+
+
+class TestSurveyElementMappingBehaviour(TestCase):
+    def tearDown(self):
+        # Undo the warnings filter set in the below test.
+        warnings.resetwarnings()
+
+    def test_get_call_patterns_equivalent_to_base_dict(self):
+        """Should find that, except for deprecated usage, SurveyElement is dict-like."""
+        # To demonstrate how dict normally works using same test cases.
+        _dict = {"name": "test", "label": None}
+        # getattr
+        self.assertEqual("default", getattr(_dict, "foo", "default"))
+        # defined key, no default
+        self.assertEqual("test", _dict.get("name"))
+        # defined key, with default
+        self.assertEqual("test", _dict.get("name", "default"))
+        # defined key, with None value
+        self.assertEqual(None, _dict.get("label"))
+        # defined key, with None value, with default
+        self.assertEqual(None, _dict.get("label", "default"))
+        # undefined key, with default
+        self.assertEqual("default", _dict.get("foo", "default"))
+        # undefined key, with default None
+        self.assertEqual(None, _dict.get("foo", None))
+        # other access patterns for undefined key
+        self.assertEqual(None, _dict.get("foo"))
+        with self.assertRaises(AttributeError):
+            _ = _dict.foo
+        with self.assertRaises(KeyError):
+            _ = _dict["foo"]
+
+        elem = SurveyElement(name="test")
+        # getattr
+        self.assertEqual("default", getattr(elem, "foo", "default"))
+        # defined key, no default
+        self.assertEqual("test", elem.get("name"))
+        # defined key, with default
+        self.assertEqual("test", elem.get("name", "default"))
+        # defined key, with None value
+        self.assertEqual(None, elem.get("label"))
+        # defined key, with None value, with default
+        self.assertEqual(None, elem.get("label", "default"))
+        # undefined key, with default
+        with self.assertWarns(DeprecationWarning) as warned:
+            self.assertEqual("default", elem.get("foo", "default"))
+        # Warning points to caller, rather than survey_element or collections.abc.
+        self.assertEqual(__file__, warned.filename)
+        # undefined key, with default None
+        with self.assertWarns(DeprecationWarning) as warned:
+            self.assertEqual(None, elem.get("foo", None))
+        # Callers can disable warnings at module-level.
+        warnings.simplefilter("ignore", DeprecationWarning)
+        with warnings.catch_warnings(record=True) as warned:
+            elem.get("foo", "default")
+        self.assertEqual(0, len(warned))
+        # other access patterns for undefined key
+        with self.assertRaises(AttributeError):
+            _ = elem.get("foo")
+        with self.assertRaises(AttributeError):
+            _ = elem.foo
+        with self.assertRaises(AttributeError):
+            _ = elem["foo"]