Repository: praw-dev/praw
Pull request #1693 (2021-02-24 15:15:11)

Use the ABC class to declare an abstract class
This allows IDEs and other tools to recognize that the class is abstract and throws the appropriate warnings.

bboe: I looked into the ABC class and decided it wasn't really necessary. Can you provide concrete examples of how using the base class actually helps with something?
PythonCoderAS: The ABC is special because it will not allow you to create a class with abstract methods not defined or marked abstract. This shows errors at initialization rather than at runtime. For example, if you forget to make the method to store the refresh token, using ABC will tell you when you first run the program. However, if you forgot to override it without the ABC class, then a runtime exception would occur, and the refresh token would get consumed with no way to get the new one.
LilSpazJoekp: You make a good point regarding losing the new refresh token if the method storing it isn't implemented. In that case, could you update the tests that failed?
bboe: Okay, I think I see some benefits now. Are we using it elsewhere for baseclasses?
PythonCoderAS: > Okay, I think I see some benefits now. Are we using it elsewhere for baseclasses?

There are no other base classes that are designed to be subclassed by 3rd parties.
PythonCoderAS: While writing a test for the ABC class, it seems that the abstract properties are checked on class initialization, so it should still be useful, since a copy is initialized when the Reddit class is created.

Patch:
diff --git a/praw/util/token_manager.py b/praw/util/token_manager.py
index 8d82003b..994fbec5 100644
--- a/praw/util/token_manager.py
+++ b/praw/util/token_manager.py
@@ -10,8 +10,10 @@ See ref:`using_refresh_tokens` for examples on how to leverage these classes.
 
 """
 
+from abc import ABC, abstractmethod
 
-class BaseTokenManager:
+
+class BaseTokenManager(ABC):
     """An abstract class for all token managers."""
 
     def __init__(self):
@@ -31,6 +33,7 @@ class BaseTokenManager:
             )
         self._reddit = value
 
+    @abstractmethod
     def post_refresh_callback(self, authorizer):
         """Handle callback that is invoked after a refresh token is used.
 
@@ -42,8 +45,8 @@ class BaseTokenManager:
         ``refresh_token``.
 
         """
-        raise NotImplementedError("``post_refresh_callback`` must be extended.")
 
+    @abstractmethod
     def pre_refresh_callback(self, authorizer):
         """Handle callback that is invoked before refreshing PRAW's authorization.
 
@@ -55,7 +58,6 @@ class BaseTokenManager:
         ``refresh_token``.
 
         """
-        raise NotImplementedError("``pre_refresh_callback`` must be extended.")
 
 
 class FileTokenManager(BaseTokenManager):


Test patch:
diff --git a/tests/unit/test_reddit.py b/tests/unit/test_reddit.py
index 7ad366cf..1f2a2adb 100644
--- a/tests/unit/test_reddit.py
+++ b/tests/unit/test_reddit.py
@@ -16,6 +16,14 @@ from praw.util.token_manager import BaseTokenManager
 from . import UnitTest
 
 
+class DummyTokenManager(BaseTokenManager):
+    def post_refresh_callback(self, authorizer):
+        pass
+
+    def pre_refresh_callback(self, authorizer):
+        pass
+
+
 class TestReddit(UnitTest):
     REQUIRED_DUMMY_SETTINGS = {
         x: "dummy" for x in ["client_id", "client_secret", "user_agent"]
@@ -202,8 +210,8 @@ class TestReddit(UnitTest):
     def test_read_only__with_authenticated_core(self):
         with Reddit(
             password=None,
-            token_manager=BaseTokenManager(),
             username=None,
+            token_manager=DummyTokenManager(),
             **self.REQUIRED_DUMMY_SETTINGS,
         ) as reddit:
             assert not reddit.read_only
@@ -230,8 +238,8 @@ class TestReddit(UnitTest):
             client_id="dummy",
             client_secret=None,
             redirect_uri="dummy",
-            token_manager=BaseTokenManager(),
             user_agent="dummy",
+            token_manager=DummyTokenManager(),
         ) as reddit:
             assert not reddit.read_only
             reddit.read_only = True
diff --git a/tests/unit/util/test_token_manager.py b/tests/unit/util/test_token_manager.py
index 5ec700ee..60c09a76 100644
--- a/tests/unit/util/test_token_manager.py
+++ b/tests/unit/util/test_token_manager.py
@@ -6,6 +6,7 @@ import pytest
 from praw.util.token_manager import BaseTokenManager, FileTokenManager
 
 from .. import UnitTest
+from ..test_reddit import DummyTokenManager
 
 
 class DummyAuthorizer:
@@ -14,25 +15,17 @@ class DummyAuthorizer:
 
 
 class TestBaseTokenManager(UnitTest):
-    def test_post_refresh_token_callback__raises_not_implemented(self):
-        manager = BaseTokenManager()
-        with pytest.raises(NotImplementedError) as excinfo:
-            manager.post_refresh_callback(None)
-        assert str(excinfo.value) == "``post_refresh_callback`` must be extended."
-
-    def test_pre_refresh_token_callback__raises_not_implemented(self):
-        manager = BaseTokenManager()
-        with pytest.raises(NotImplementedError) as excinfo:
-            manager.pre_refresh_callback(None)
-        assert str(excinfo.value) == "``pre_refresh_callback`` must be extended."
+    def test_init_base_fail(self):
+        with pytest.raises(TypeError):
+            BaseTokenManager()
 
     def test_reddit(self):
-        manager = BaseTokenManager()
+        manager = DummyTokenManager()
         manager.reddit = "dummy"
         assert manager.reddit == "dummy"
 
     def test_reddit__must_only_be_set_once(self):
-        manager = BaseTokenManager()
+        manager = DummyTokenManager()
         manager.reddit = "dummy"
         with pytest.raises(RuntimeError) as excinfo:
             manager.reddit = None