Merge pull request #108 from rohanpm/erratum-from

erratum: rename "from_" to "from" (with alias available)

CHANGELOG.md

@@ -7,7 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-- n/a
+### Changed
+- On `ErratumPushItem`, the `from_` attribute is now available under the preferred
+  name of `from`. Since this clashes with the python keyword of the same name, the
+  `from_` name will remain available indefinitely.
 
 ## [2.1.0] - 2020-11-12
 

src/pushsource/_impl/model/erratum.py

@@ -12,6 +12,7 @@
     optional,
     optional_str,
 )
+from .erratum_fixup import fixup_erratum_class
 
 
 @attr.s()
@@ -231,7 +232,12 @@ class ErratumPushItem(PushItem):
     associated with the advisory."""
 
     from_ = attr.ib(type=str, default=None, validator=optional_str)
-    """Contact email address for the owner of the advisory."""
+    """Contact email address for the owner of the advisory.
+
+    Note that the canonical name for this attribute is ``from``. As this clashes
+    with a Python keyword, in most contexts the attribute is also available as an
+    alias, ``from_``. Where possible, the canonical name ``from`` should be preferred.
+    """
 
     rights = attr.ib(type=str, default=None, validator=optional_str)
     """Copyright message."""
@@ -308,6 +314,7 @@ def _from_data(cls, data):
             "reboot_suggested",
             "rights",
             "title",
+            "from",
             "description",
             "version",
             "updated",
@@ -318,9 +325,6 @@ def _from_data(cls, data):
         ]:
             kwargs[field] = data[field]
 
-        # Workaround to avoid python keyword
-        kwargs["from_"] = data["from"]
-
         # If the metadata has a cdn_repo field, this sets the destinations for the
         # push item; used for text-only errata.
         if data.get("cdn_repo"):
@@ -339,3 +343,7 @@ def _from_data(cls, data):
         )
 
         return cls(**kwargs)
+
+
+# Apply from vs from_ workarounds.
+fixup_erratum_class(ErratumPushItem)

src/pushsource/_impl/model/erratum_fixup.py

@@ -0,0 +1,129 @@
+"""Helpers to deal with "from" vs "from_" issue in ErratumPushItem.
+
+The advisory model has a "from" field, which unfortunately clashes with
+a Python keyword of the same name. This prevents directly declaring the
+field as "from".
+
+Despite the clash, it is desirable to have the attrs field named "from"
+so that usage of this model does not have to be frequently accompanied
+by some "from_" => "from" renaming (e.g. before uploading to Pulp).
+
+It is possible to make this work by:
+
+- in the ErratumPushItem class, declare the attribute as "from_"
+- then, after the class is created, patch it to effectively rename it
+  to "from"
+
+This makes use of the __attrs_attrs__ attribute which is associated
+with each attrs class. Though the name implies it is private, this is
+a supported & documented method of extending the behavior of attrs,
+see: https://www.attrs.org/en/stable/extending.html
+
+A simpler trick may come to mind: just setattr(ErratumPushItem, 'from', attr.ib(...))
+to work around the inability to write "from = attr.ib(...)".
+Unfortunately, that does not work because the attrs library internally
+tries to eval some code of the form "<attr_name> = ...", which will never
+work if the name is a keyword.
+"""
+
+
+class AttrsRenamer(object):
+    def __init__(self, delegate, attrs_old_to_new):
+        """Helper to selectively rename certain attributes for __attrs_attrs__
+        within a class.
+
+        Arguments:
+            delegate (tuple)
+                Original instance of __attrs_attrs__, generated by attrs library.
+            attrs_old_to_new (dict)
+                A mapping from old to new attribute names.
+        """
+        self._delegate = delegate
+        self._attrs_old_to_new = attrs_old_to_new
+
+        self._attrs_by_name = {}
+        for old_name, new_name in self._attrs_old_to_new.items():
+            old_attr = getattr(delegate, old_name)
+
+            # Make a new attribute, identical to old in all respects
+            # except for the name.
+            # (we do this dynamically to cope with differences between ancient and newer
+            # versions of attrs library)
+            attr_kwargs = {"name": new_name}
+            for argname in [
+                "default",
+                "validator",
+                "repr",
+                "cmp",
+                "hash",
+                "init",
+                "inherited",
+            ]:
+                if hasattr(old_attr, argname):
+                    attr_kwargs[argname] = getattr(old_attr, argname)
+            new_attr = old_attr.__class__(**attr_kwargs)
+
+            self._attrs_by_name[new_name] = new_attr
+
+    def __iter__(self):
+        for elem in self._delegate:
+            new_name = self._attrs_old_to_new.get(elem.name)
+            if new_name:
+                # Something we've renamed - yield ours
+                yield self._attrs_by_name[new_name]
+            else:
+                # Something else - yield original
+                yield elem
+
+    def __getattr__(self, name):
+        # If it's one of the new attribute names, just return it directly
+        if name in self._attrs_by_name:
+            return self._attrs_by_name[name]
+
+        # If it's one of the old attribute names, make it not exist
+        if name in self._attrs_old_to_new:
+            raise AttributeError()
+
+        # If it's anything else then just let the delegate handle it,
+        # giving exactly normal behavior
+        return getattr(self._delegate, name)
+
+
+def fixup_attrs(cls):
+    # Fix up the set of attributes on the class.
+    #
+    # This impacts dynamic use of the class e.g. via attr.fields, attr.asdict,
+    # attr.evolve, ...
+    cls.__attrs_attrs__ = AttrsRenamer(cls.__attrs_attrs__, {"from_": "from"})
+
+
+def fixup_init(cls):
+    # Fix up the constructor of the class to support both names
+    # of the "from" attribute.
+    #
+    # As the underlying attribute is actually stored as "from_", we need to patch
+    # the constructor to accept the "from" argument too and rename it for storage.
+    #
+    # If the caller provides both "from" and "from_", the former is preferred.
+    cls_init = cls.__init__
+
+    def new_init(*args, **kwargs):
+        if "from" in kwargs:
+            kwargs["from_"] = kwargs.pop("from") or kwargs.get("from_")
+        return cls_init(*args, **kwargs)
+
+    cls.__init__ = new_init
+
+
+def fixup_props(cls):
+    # Fix up properties of the class so that there is a "from" property which
+    # aliases the underlying "from_".
+    from_attr = property(lambda self: self.from_)
+    setattr(cls, "from", from_attr)
+
+
+def fixup_erratum_class(cls):
+    # Do all the fixups to make "from" and "from_" aliases.
+    fixup_attrs(cls)
+    fixup_init(cls)
+    fixup_props(cls)

tests/model/test_errata_from.py

@@ -0,0 +1,88 @@
+"""Tests for special handling of "from" field on errata."""
+from pytest import raises
+import attr
+
+from pushsource import ErratumPushItem
+
+
+def test_construct_underscore():
+    """Constructing item with 'from_' works correctly."""
+    item = ErratumPushItem(name="TEST-123", from_="test-from")
+
+    # Should be identical under both names
+    assert item.from_ == "test-from"
+    assert getattr(item, "from") == "test-from"
+
+
+def test_construct_nounderscore():
+    """Constructing item with 'from' works correctly."""
+    kwargs = {"from": "test-from"}
+    item = ErratumPushItem(name="TEST-123", **kwargs)
+
+    # Should be identical under both names
+    assert item.from_ == "test-from"
+    assert getattr(item, "from") == "test-from"
+
+
+def test_construct_mixed():
+    """Constructing item with both 'from' and 'from_' works correctly,
+    with 'from' being preferred."""
+    kwargs = {"from": "from1", "from_": "from2"}
+    item = ErratumPushItem(name="TEST-123", **kwargs)
+
+    # Should be identical under both names - 'from_' is just discarded
+    assert item.from_ == "from1"
+    assert getattr(item, "from") == "from1"
+
+
+def test_evolve_nounderscore():
+    """Evolving item with "from" works correctly."""
+    kwargs = {"from": "test-from"}
+    item = ErratumPushItem(name="TEST-123")
+    item = attr.evolve(item, **kwargs)
+
+    # Should be identical under both names
+    assert item.from_ == "test-from"
+    assert getattr(item, "from") == "test-from"
+
+
+def test_evolve_underscore():
+    """Evolving item with "from_" works correctly."""
+    item = ErratumPushItem(name="TEST-123")
+    item = attr.evolve(item, from_="test-from")
+
+    # Should be identical under both names
+    assert item.from_ == "test-from"
+    assert getattr(item, "from") == "test-from"
+
+
+def test_fields():
+    """Item class has "from" field and not "from_" field."""
+    fields = attr.fields(ErratumPushItem)
+
+    # It should have a field named "from" with correct name.
+    assert hasattr(fields, "from")
+    assert getattr(fields, "from").name == "from"
+
+    # It should not have any "from_" field (as this is considered
+    # merely an alias, not a proper field).
+    assert not hasattr(fields, "from_")
+
+    # Other unrelated fields should work as normal.
+    assert hasattr(fields, "status")
+    assert fields.status.name == "status"
+
+
+def test_asdict():
+    """asdict() returns "from" and not "from_"."""
+    kwargs = {"name": "adv", "from": "bob"}
+    item = ErratumPushItem(**kwargs)
+
+    item_dict = attr.asdict(item)
+
+    # It should have exactly the fields from the inputs
+    assert item_dict["name"] == kwargs["name"]
+    assert item_dict["from"] == kwargs["from"]
+
+    # And it should not have any extra 'from_'
+    assert "from_" not in item_dict