Repository: repobee/repobee

Issue:
APIObject should raise when accessing the internal implementation, if it is None
As APIObjects are often passed around without the `implementation` attribute being initialized, it would be good if an exception was raised if it is accessed when not initialized. If an APIObject is built from an API platform-specific object, it should _always_ have an initialized `implementation` attribute. If it is not built from an API platform-specific object, then there should never be a reason to access the `implementation` attribute.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=h1) Report
> Merging [#235](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/293665ce1ec44fc274806ae7d002bd06bd932c2d?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/235/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master    #235      +/-   ##
=========================================
+ Coverage   91.37%   91.4%   +0.03%     
=========================================
  Files          19      19              
  Lines        1310    1315       +5     
  Branches      237     238       +1     
=========================================
+ Hits         1197    1202       +5     
  Misses         93      93              
  Partials       20      20
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/apimeta.py](https://codecov.io/gh/repobee/repobee/pull/235/diff?src=pr&el=tree#diff-cmVwb2JlZS9hcGltZXRhLnB5) | `92.85% <100%> (+0.45%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=footer). Last update [293665c...c0b9fc1](https://codecov.io/gh/repobee/repobee/pull/235?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/repobee/apimeta.py b/repobee/apimeta.py
index b1b2f7d..5b92c54 100644
--- a/repobee/apimeta.py
+++ b/repobee/apimeta.py
@@ -35,6 +35,25 @@ MAX_NAME_LENGTH = 100
 class APIObject:
     """Base wrapper class for platform API objects."""
 
+    def __getattribute__(self, name: str):
+        """If the sought attr is 'implementation', and that attribute is None,
+        an AttributeError should be raise. This is because there should never
+        be a case where the caller tries to access a None implementation: if
+        it's None the caller should now without checking, as only API objects
+        returned by platform API (i.e. a class deriving from :py:class:`API`)
+        can have a reasonable value for the implementation attribute.
+
+        In all other cases, proceed as usual in getting the attribute. This
+        includes the case when ``name == "implementation"``, and the APIObject
+        does not have that attribute.
+        """
+        attr = object.__getattribute__(self, name)
+        if attr is None and name == "implementation":
+            raise AttributeError(
+                "invalid access to 'implementation': not initialized"
+            )
+        return attr
+
 
 def _check_name_length(name):
     """Check that a Team/Repository name does not exceed the maximum GitHub


Test patch:
diff --git a/tests/test_apimeta.py b/tests/test_apimeta.py
index acb4b9d..a6bb707 100644
--- a/tests/test_apimeta.py
+++ b/tests/test_apimeta.py
@@ -2,6 +2,8 @@ import pytest
 from repobee import apimeta
 from repobee import exception
 
+import collections
+
 
 def api_methods():
     methods = apimeta.methods(apimeta.APISpec.__dict__)
@@ -14,47 +16,84 @@ def api_method_ids():
     return list(methods.keys())
 
 
-@pytest.mark.parametrize("method", api_methods(), ids=api_method_ids())
-def test_raises_when_unimplemented_method_called(method):
-    """Test that get_teams method raises NotImplementedError when called if
-    left undefined.
-    """
+class TestAPI:
+    @pytest.mark.parametrize("method", api_methods(), ids=api_method_ids())
+    def test_raises_when_unimplemented_method_called(self, method):
+        """Test that get_teams method raises NotImplementedError when called if
+        left undefined.
+        """
+
+        class API(apimeta.API):
+            pass
 
-    class API(apimeta.API):
-        pass
+        name, impl = method
+        params = apimeta.parameter_names(impl)
 
-    name, impl = method
-    params = apimeta.parameter_names(impl)
+        with pytest.raises(NotImplementedError):
+            m = getattr(API, name)
+            arguments = (None,) * len(params)
+            m(*arguments)
 
-    with pytest.raises(NotImplementedError):
-        m = getattr(API, name)
-        arguments = (None,) * len(params)
-        m(*arguments)
+    def test_raises_when_method_incorrectly_declared(self):
+        """``get_teams`` takes only a self argument, so defining it with a
+        different argument should raise.
+        """
 
+        with pytest.raises(exception.APIImplementationError):
 
-def test_raises_when_method_incorrectly_declared():
-    """``get_teams`` takes only a self argument, so defining it with a
-    different argument should raise.
-    """
+            class API(apimeta.API):
+                def get_teams(a):
+                    pass
 
-    with pytest.raises(exception.APIImplementationError):
+    def test_accepts_correctly_defined_method(self):
+        """API should accept a correctly defined method, and not alter it in any
+        way.
+        """
+        expected = 42
 
         class API(apimeta.API):
-            def get_teams(a):
+            def __init__(self, base_url, token, org_name, user):
                 pass
 
+            def get_teams(self):
+                return expected
 
-def test_accepts_correctly_defined_method():
-    """API should accept a correctly defined method, and not alter it in any
-    way.
-    """
-    expected = 42
+        assert API(None, None, None, None).get_teams() == expected
 
-    class API(apimeta.API):
-        def __init__(self, base_url, token, org_name, user):
-            pass
 
-        def get_teams(self):
-            return expected
+class TestAPIObject:
+    def test_raises_when_accessing_none_implementation(self):
+        """Any APIObject should raise when the implementation attribute is
+        accessed, if it is None.
+        """
+
+        class APIObj(
+            apimeta.APIObject,
+            collections.namedtuple("APIObj", "implementation"),
+        ):
+            def __new__(cls):
+                return super().__new__(cls, implementation=None)
+
+        obj = APIObj()
+
+        with pytest.raises(AttributeError) as exc_info:
+            obj.implementation
+
+        assert "invalid access to 'implementation': not initialized" in str(
+            exc_info
+        )
+
+    def test_does_not_raise_when_accessing_initialized_implementation(self):
+        """If implementation is not None, there should be no error on access"""
+        implementation = 42
+
+        class APIObj(
+            apimeta.APIObject,
+            collections.namedtuple("APIObj", "implementation"),
+        ):
+            def __new__(cls):
+                return super().__new__(cls, implementation=implementation)
+
+        obj = APIObj()
 
-    assert API(None, None, None, None).get_teams() == expected
+        assert obj.implementation == implementation