SAR-149 Resolve some review comments

- Pass parameters as a keyword argument to transaction
- Fix test to reflect change to transaction module

src/ska/logging/transactions.py

@@ -5,7 +5,7 @@ import json
 import logging
 import os
 
-from typing import Mapping, Text
+from typing import Text
 
 from ska.skuid.client import SkuidClient
 
@@ -31,15 +31,13 @@ class Transaction:
 
      """
 
-    def __init__(self, name, params, logger=None):
-        if not isinstance(params, Mapping):
-            raise TransactionParamsError("params must be dict-like (Mapping)")
-        self.logger = logger
-        if not logger:
+    def __init__(self, name, **params):
+        # TODO: get the logger from the transaction logginh application without passing to 
+        # context handler see https://gitlab.com/ska-telescope/ska-logging/-/merge_requests/7#note_414359589
         self.logger = logging.getLogger(__name__)
         self._name = name
         self._params = params
-        self.transaction_id = self._get_existing_transaction_id() or self._generate_new_id()
+        self.transaction_id = self._get_new_or_existing_transaction_id()
 
     def __enter__(self):
         params_json = json.dumps(self._params)
@@ -56,10 +54,10 @@ class Transaction:
         if exc_type:
             raise
 
-    def _get_existing_transaction_id(self):
+    def _get_new_or_existing_transaction_id(self):
         transaction_id = self._params.get("transaction_id")
         if not self._is_valid_id(transaction_id):
-            transaction_id = None
+            transaction_id = self._generate_new_id()
         return transaction_id
 
     def _is_valid_id(self, transaction_id):
@@ -87,7 +85,3 @@ class TransactionIdGenerator:
 
     def next(self):
         return self._get_id()
-
-
-class TransactionParamsError(TypeError):
-    """Invalid data type for transaction parameters."""

tests/test_transactions.py

@@ -10,7 +10,7 @@ import concurrent.futures
 from unittest.mock import patch, MagicMock
 
 from ska.logging import transaction
-from ska.logging.transactions import TransactionParamsError, TransactionIdGenerator
+from ska.logging.transactions import TransactionIdGenerator
 from tests.conftest import (
     get_first_record_and_log_message,
     get_last_record_and_log_message,
@@ -23,62 +23,56 @@ class TestTransactionIdGeneration:
 
     def test_existing_valid_id_is_reused(self, id_generator_stub):
         parameters = {"transaction_id": "abc1234", "other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == "abc1234"
 
     def test_new_id_generated_if_id_is_empty(self, id_generator_stub):
         parameters = {"transaction_id": "", "other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_new_id_generated_if_id_is_none(self, id_generator_stub):
         parameters = {"transaction_id": None, "other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_new_id_generated_if_id_is_only_white_space(self, id_generator_stub):
         parameters = {"transaction_id": "\t\n \r\n", "other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_new_id_generated_if_id_is_not_string_type(self, id_generator_stub):
         parameters = {"transaction_id": 1234.5, "other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_new_id_generated_if_id_is_not_present(self, id_generator_stub):
         parameters = {"other": "config"}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_new_id_generated_if_params_empty(self, id_generator_stub):
         parameters = {}
-        with transaction("name", parameters) as transaction_id:
+        with transaction("name", **parameters) as transaction_id:
             assert transaction_id == id_generator_stub.last_id
 
     def test_id_provider_only_used_once_for_one_new_id(self, id_generator_stub):
         parameters = {}
-        with transaction("name", parameters):
+        with transaction("name", **parameters):
             assert id_generator_stub.call_count == 1
 
     def test_id_provider_not_used_for_existing_valid_id(self, id_generator_stub):
         parameters = {"transaction_id": "abc1234"}
-        with transaction("name", parameters):
+        with transaction("name", **parameters):
             assert id_generator_stub.call_count == 0
 
-    def test_error_if_params_type_is_not_mapping(self):
-        parameters = []
-        with pytest.raises(TransactionParamsError):
-            with transaction("name", parameters):
-                pass
-
 
 class TestTransactionLogging:
     """Tests for :class:`~ska.logging.transaction` related to logging."""
 
     def test_name_and_id_and_params_in_context_handler(self, id_generator_stub, recording_logger):
-        params = {"other": ["config", 1, 2, 3.0]}
-        with transaction("name", params) as transaction_id:
+        parameters = {"other": ["config", 1, 2, 3.0]}
+        with transaction("name", **parameters) as transaction_id:
             pass
         _, first_log_message = get_first_record_and_log_message(recording_logger)
         _, last_log_message = get_last_record_and_log_message(recording_logger)
@@ -87,7 +81,7 @@ class TestTransactionLogging:
         assert "Start transaction" in first_log_message
         assert "name" in first_log_message
         assert transaction_id in first_log_message
-        assert json.dumps(params) in first_log_message
+        assert json.dumps(parameters) in first_log_message
 
         # __exit__ log message
         assert "Exit transaction" in last_log_message
@@ -95,9 +89,9 @@ class TestTransactionLogging:
         assert transaction_id in last_log_message
 
     def test_exception_logs_transaction_id_and_command(self, id_generator_stub, recording_logger):
-        params = {"other": ["config", 1, 2, 3.0]}
+        parameters = {"other": ["config", 1, 2, 3.0]}
         with pytest.raises(RuntimeError):
-            with transaction("name", params) as transaction_id:
+            with transaction("name", **parameters) as transaction_id:
                 raise RuntimeError("Something went wrong")
 
         record_logs = get_all_record_logs(recording_logger)