Repository: ImperialCollegeLondon/guikit (language: python)

Issue:
Simplify thread system
Using the thread system should be really, really easy to encourage its use. The backend is probably fine as it is, but the user facing side should probably be simplified.  

The most straight forward solution would be just to use alias for the most common operations:

- [ ] `run_in_thread` function for `ThreadPool().run_thread`
- [ ] `abort_thread` function for `ThreadPool().abort_thread`
- [ ] `should_abort` function for `ThreadPool().query_abort`
- [ ] `run_daemon`, shortcut for running a function in a daemonic thread


Discussion / hints:

Fix patch:
diff --git a/pyguitemp/threads.py b/pyguitemp/threads.py
index 73cf080..2e5c538 100644
--- a/pyguitemp/threads.py
+++ b/pyguitemp/threads.py
@@ -8,6 +8,8 @@
 
 import wx
 
+from .logging import logger
+
 
 class ThreadResult(wx.PyEvent):
     """Simple event to carry arbitrary result data."""
@@ -171,17 +173,71 @@ def run_thread(
         return thread.ident
 
     def query_abort(self) -> bool:
-        """Checks if the current thread is to be aborted."""
-        return self._workers[threading.get_ident()].abort
+        """Check if the current thread is to be aborted.
+
+        Raises:
+            KeyError: If the thread identifier is not in the ThreadPool
+        """
+        ident: int = threading.get_ident()
+
+        try:
+            return self._workers[ident].abort
+        except KeyError as key_err:
+            key_err_ = KeyError(f"Thread with index: {ident} is not in the ThreadPool.")
+            logger.exception(key_err_)
+            raise key_err_ from key_err
 
     def abort_thread(self, ident: int) -> None:
-        """Flags the thread with `ident` to be aborted.
+        """Flag the thread with `ident` to be aborted.
 
         Args:
-            ident: The id of the thread
+            ident: Thread identifier
+
+        Raises:
+            KeyError: If the thread identifier is not in the ThreadPool
         """
-        self._workers[ident].abort = True
+        try:
+            self._workers[ident].abort = True
+        except KeyError as key_err:
+            key_err_ = KeyError(f"Thread with index: {ident} is not in the ThreadPool.")
+            logger.exception(key_err_)
+            raise key_err_ from key_err
 
     def post_event(self, event: ThreadResult):
         """Adds an event to the event loop of the main thread."""
         wx.PostEvent(self._window, event)
+
+
+def run_in_thread(
+    target: Callable,
+    on_abort: Optional[Callable] = None,
+    on_complete: Optional[Callable] = None,
+    on_error: Optional[Callable] = None,
+    daemon: Optional[bool] = None,
+) -> int:
+    """Is an alias for ThreadPool().run_thread(...)."""
+    return ThreadPool().run_thread(target, on_abort, on_complete, on_error, daemon)
+
+
+def run_daemon(
+    target: Callable,
+    on_abort: Optional[Callable] = None,
+    on_complete: Optional[Callable] = None,
+    on_error: Optional[Callable] = None,
+) -> int:
+    """Is an alias for ThreadPool().run_thread(..., daemon=True)."""
+    return ThreadPool().run_thread(target, on_abort, on_complete, on_error, daemon=True)
+
+
+def abort_thread(ident: int) -> None:
+    """Set thread with given identifier to abort.
+
+    Args:
+        ident: Thread identifier
+    """
+    ThreadPool().abort_thread(ident)
+
+
+def should_abort() -> bool:
+    """Return whether the thread with given identifier should abort or not."""
+    return ThreadPool().query_abort()


Test patch:
diff --git a/tests/test_threads.py b/tests/test_threads.py
index 9f93fc4..ea8cd4a 100644
--- a/tests/test_threads.py
+++ b/tests/test_threads.py
@@ -1,6 +1,6 @@
 from unittest.mock import MagicMock, patch
 
-from pytest import mark
+from pytest import mark, raises
 
 
 def test_thread_result():
@@ -71,13 +71,15 @@ def data(self):
         event._data.assert_called_once()
 
 
+class Worker:
+    ident = 42
+    abort = False
+    connect_events = MagicMock()
+    start = MagicMock()
+
+
 class TestThreadPool:
     def test_run_thread(self, window):
-        class Worker:
-            ident = 42
-            abort = False
-            connect_events = MagicMock()
-            start = MagicMock()
 
         with patch("pyguitemp.threads.WorkerThread", MagicMock(return_value=Worker)):
             from pyguitemp.threads import ThreadPool
@@ -88,39 +90,45 @@ class Worker:
         assert Worker.ident in pool._workers
 
     def test_query_abort(self, window):
-        import threading
-
-        class Worker:
-            ident = threading.get_ident()
-            abort = False
-            connect_events = MagicMock()
-            start = MagicMock()
-
-        with patch("pyguitemp.threads.WorkerThread", MagicMock(return_value=Worker)):
-            from pyguitemp.threads import ThreadPool
+        with patch(
+            "pyguitemp.threads.WorkerThread", MagicMock(return_value=Worker)
+        ), patch("pyguitemp.threads.logger", MagicMock()), patch(
+            "pyguitemp.threads.threading.get_ident",
+            MagicMock(return_value=Worker.ident + 1),
+        ):
+            from pyguitemp.threads import ThreadPool, logger
 
             pool = ThreadPool(window)
-            pool.run_thread(lambda: None)
-
-        assert not pool.query_abort()
 
-    def test_abort_thread(self, window):
-        import threading
+            with raises(KeyError) as key_err:
+                pool.query_abort()
 
-        class Worker:
-            ident = threading.get_ident()
-            abort = False
-            connect_events = MagicMock()
-            start = MagicMock()
+            key_err_str = str(key_err.value)
+            assert (
+                f"Thread with index: {Worker.ident+1} is not in the ThreadPool."
+                in key_err_str
+            )
+            assert logger.exception.call_count == 1
 
-        with patch("pyguitemp.threads.WorkerThread", MagicMock(return_value=Worker)):
-            from pyguitemp.threads import ThreadPool
+    def test_abort_thread(self, window):
+        with patch(
+            "pyguitemp.threads.WorkerThread", MagicMock(return_value=Worker)
+        ), patch("pyguitemp.threads.logger", MagicMock()):
+            from pyguitemp.threads import ThreadPool, logger
 
             pool = ThreadPool(window)
-            ident = pool.run_thread(lambda: None)
 
-        pool.abort_thread(ident)
-        assert pool._workers[ident].abort
+            with raises(KeyError) as key_err:
+                pool.abort_thread(Worker.ident + 1)
+            key_err_str = str(key_err.value)
+            assert (
+                f"Thread with index: {Worker.ident+1} is not in the ThreadPool."
+                in key_err_str
+            )
+            assert logger.exception.call_count == 1
+
+        pool.abort_thread(Worker.ident)
+        assert pool._workers[Worker.ident].abort
 
     def test_post_event(self, window):
         class WX:
@@ -132,3 +140,39 @@ class WX:
             pool = ThreadPool(window)
             pool.post_event(None)
             WX.PostEvent.assert_called_once()
+
+
+def test_run_in_thread():
+    with patch("pyguitemp.threads.ThreadPool", MagicMock()):
+        from pyguitemp.threads import ThreadPool, run_in_thread
+
+        def target():
+            pass
+
+        run_in_thread(target)
+        assert ThreadPool().run_thread.call_count == 1
+        ThreadPool().run_thread.assert_called_once_with(target, None, None, None, None)
+
+
+def test_run_daemon():
+    with patch("pyguitemp.threads.ThreadPool", MagicMock()):
+        from pyguitemp.threads import ThreadPool, run_daemon
+
+        run_daemon(lambda: None)
+        ThreadPool().run_daemon_called_once_with(None, None, None, None, True)
+
+
+def test_abort_thread():
+    with patch("pyguitemp.threads.ThreadPool", MagicMock()):
+        from pyguitemp.threads import ThreadPool, abort_thread
+
+        abort_thread(43)
+        ThreadPool().abort_thread.called_once_with(43)
+
+
+def test_should_abort():
+    with patch("pyguitemp.threads.ThreadPool", MagicMock()):
+        from pyguitemp.threads import ThreadPool, should_abort
+
+        should_abort()
+        ThreadPool().query_abort.called_once_with()