Merge pull request OpenAssetIO#537 from foundrytom/work/513-logArgume…

…ntOrder

[Core] Swap `LoggerInterface::log` arguments

RELEASE_NOTES.md

@@ -6,6 +6,10 @@ v1.0.0-alpha.X
 
 ### Breaking changes
 
+- Swapped the order of `severity` and `message` in the
+  `LoggerInterface::log` method.
+  [#531](https://github.com/OpenAssetIO/OpenAssetIO/issues/531)
+
 - Removed the `Session` Python class in favour of the `ManagerFactory`
   C++/Python class.
   [#430](https://github.com/OpenAssetIO/OpenAssetIO/issues/430)

python/openassetio/log.py

@@ -92,15 +92,15 @@ def getSeverity(self):
 
     # LoggerInterface methods
 
-    def log(self, message, severity):
+    def log(self, severity, message):
         """
         Log only if `severity` is greater than or equal to this logger's
         configured severity level.
         """
         if severity > self.__maxSeverity:
             return
 
-        self.__upstreamLogger.log(message, severity)
+        self.__upstreamLogger.log(severity, message)
 
 
 class ConsoleLogger(LoggerInterface):
@@ -134,7 +134,7 @@ def __init__(self, colorOutput=True, forceDefaultStreams=False):
             if not sys.__stderr__.closed:
                 self.__stderr = sys.__stderr__
 
-    def log(self, message, severity):
+    def log(self, severity, message):
         """
         Log to stderr for severity greater than `kInfo`, stdout
         otherwise.

python/openassetio/managerApi/HostSession.py

@@ -64,7 +64,7 @@ def __init__(self, host, logger):
 
         self.__logger = logger
 
-    def log(self, message, severity):
+    def log(self, severity, message):
         """
         Logs a message to the user.
 
@@ -74,4 +74,4 @@ def log(self, message, severity):
 
         @see @ref openassetio.log "log"
         """
-        self.__logger.log(message, severity)
+        self.__logger.log(severity, message)

python/openassetio/pluginSystem/PluginSystem.py

@@ -71,13 +71,13 @@ def scan(self, paths):
         """
         self.reset()
 
-        self.__logger.log("PluginSystem: Searching %s" % paths, self.__logger.kDebug)
+        self.__logger.log(self.__logger.kDebug, "PluginSystem: Searching %s" % paths)
 
         for path in paths.split(os.pathsep):
 
             if not os.path.isdir(path):
                 msg = "PluginSystem: Skipping as it is not a directory %s" % path
-                self.__logger.log(msg, self.__logger.kDebug)
+                self.__logger.log(self.__logger.kDebug, msg)
 
             for item in os.listdir(path):
 
@@ -91,19 +91,19 @@ def scan(self, paths):
                     else:
                         msg = "PluginSystem: Ignoring as it is not a python package " \
                               "contianing __init__.py %s" % itemPath
-                        self.__logger.log(msg, self.__logger.kDebug)
+                        self.__logger.log(self.__logger.kDebug, msg)
                         continue
                 else:
                     # Its a file, check if it is a .py/.pyc module
                     _, ext = os.path.splitext(itemPath)
                     if ext not in self.__validModuleExtensions:
                         msg = "PluginSystem: Ignoring as its not a python module %s" % itemPath
-                        self.__logger.log(msg, self.__logger.kDebug)
+                        self.__logger.log(self.__logger.kDebug, msg)
                         continue
 
                 self.__logger.log(
-                    "PluginSystem: Attempting to load %s" % itemPath,
-                    self.__logger.kDebug)
+                    self.__logger.kDebug,
+                    "PluginSystem: Attempting to load %s" % itemPath)
 
                 self.__load(itemPath)
 
@@ -153,11 +153,11 @@ def register(self, cls, path="<unknown>"):
         if identifier in self.__map:
             msg = "PluginSystem: Skipping class '%s' defined in '%s'. Already registered by '%s'" \
                   % (cls, path, self.__paths[identifier])
-            self.__logger.log(msg, self.__logger.kDebug)
+            self.__logger.log(self.__logger.kDebug, msg)
             return
 
         msg = "PluginSystem: Registered plug-in '%s' from '%s'" % (cls, path)
-        self.__logger.log(msg, self.__logger.kDebug)
+        self.__logger.log(self.__logger.kDebug, msg)
 
         self.__map[identifier] = cls
         self.__paths[identifier] = path
@@ -191,12 +191,12 @@ def __load(self, path):
 
         except Exception as ex:  # pylint: disable=broad-except
             msg = "PluginSystem: Caught exception loading plug-in from %s:\n%s" % (path, ex)
-            self.__logger.log(msg, self.__logger.kWarning)
+            self.__logger.log(self.__logger.kWarning, msg)
             return
 
         if not hasattr(module, 'plugin'):
             msg = "PluginSystem: No top-level 'plugin' variable %s" % path
-            self.__logger.log(msg, self.__logger.kWarning)
+            self.__logger.log(self.__logger.kWarning, msg)
             return
 
         # Store where this plugin was loaded from. Not entirely

python/openassetio/pluginSystem/PluginSystemManagerImplementationFactory.py

@@ -65,8 +65,8 @@ def __scan(self):
             self.__paths = os.environ.get(self.kPluginEnvVar, "")
             if not self.__paths:
                 self._logger.log(
-                    ("%s is not set. It is somewhat unlikely that you will "
-                     + "find any plugins...") % self.kPluginEnvVar, self._logger.kWarning)
+                    self._logger.kWarning, ("%s is not set. It is somewhat unlikely that you will "
+                     + "find any plugins...") % self.kPluginEnvVar)
 
         self.__pluginManager = PluginSystem(self._logger)
 
@@ -100,7 +100,7 @@ def instantiate(self, identifier):
         if not self.__pluginManager:
             self.__scan()
 
-        self._logger.log(f"Instantiating {identifier}", self._logger.kDebug)
+        self._logger.log(self._logger.kDebug, f"Instantiating {identifier}")
         plugin = self.__pluginManager.plugin(identifier)
         interface = plugin.interface()
 

resources/examples/manager/BasicAssetLibrary/plugin/BasicAssetLibrary/BasicAssetLibraryInterface.py

@@ -72,16 +72,16 @@ def initialize(self, managerSettings, hostSession):
 
         if self.__settings.get("library_path") is None:
             hostSession.log(
-                f"'library_path' not in settings, checking {self.__lib_path_envvar_name}",
                 hostSession.kDebug,
+                f"'library_path' not in settings, checking {self.__lib_path_envvar_name}",
             )
             self.__settings["library_path"] = os.environ.get(self.__lib_path_envvar_name)
 
         if self.__settings.get("library_path") is None:
             raise PluginError("'library_path' not set")
 
         hostSession.log(
-            f"Loading library from {self.__settings['library_path']}", hostSession.kDebug
+            hostSession.kDebug, f"Loading library from {self.__settings['library_path']}"
         )
         self.__library = bal.load_library(self.__settings["library_path"])
 

src/openassetio-core/include/openassetio/LoggerInterface.hpp

@@ -38,7 +38,7 @@ class OPENASSETIO_CORE_EXPORT LoggerInterface {
    * @param severity One of the severity constants defined in @ref
    * Severity.
    */
-  virtual void log(const Str& message, Severity severity) const = 0;
+  virtual void log(Severity severity, const Str& message) const = 0;
 };
 }  // namespace OPENASSETIO_CORE_ABI_VERSION
 }  // namespace openassetio

src/openassetio-python/LoggerInterfaceBinding.cpp

@@ -15,8 +15,8 @@ inline namespace OPENASSETIO_CORE_ABI_VERSION {
 struct PyLoggerInterface : LoggerInterface {
   using LoggerInterface::LoggerInterface;
 
-  void log(const Str& message, Severity severity) const override {
-    PYBIND11_OVERRIDE_PURE(void, LoggerInterface, log, message, severity);
+  void log(Severity severity, const Str& message) const override {
+    PYBIND11_OVERRIDE_PURE(void, LoggerInterface, log, severity, message);
   }
 };
 }  // namespace OPENASSETIO_CORE_ABI_VERSION
@@ -45,5 +45,5 @@ void registerLoggerInterface(const py::module& mod) {
   loggerInterface.def_readonly_static("kSeverityNames", &LoggerInterface::kSeverityNames);
 
   loggerInterface.def(py::init())
-      .def("log", &LoggerInterface::log, py::arg("message"), py::arg("severity"));
+      .def("log", &LoggerInterface::log, py::arg("severity"), py::arg("message"));
 }

tests/python/conftest.py

@@ -310,5 +310,5 @@ def __init__(self):
         LoggerInterface.__init__(self)
         self.mock = mock.create_autospec(LoggerInterface, spec_set=True, instance=True)
 
-    def log(self, message, severity):
-        self.mock.log(message, severity)
+    def log(self, severity, message):
+        self.mock.log(severity, message)

tests/python/openassetio/managerApi/test_hostsession.py

@@ -63,5 +63,5 @@ def test_forwards_to_logger(self, host_session, mock_logger):
         a_message = "A message"
         a_severity = log.LoggerInterface.kCritical
 
-        host_session.log(a_message, a_severity)
-        mock_logger.mock.log.assert_called_once_with(a_message, a_severity)
+        host_session.log(a_severity, a_message)
+        mock_logger.mock.log.assert_called_once_with(a_severity, a_message)

tests/python/openassetio/pluginSystem/test_pluginsystemmanagerimplementationfactory.py

@@ -94,7 +94,7 @@ def test_when_env_var_not_set_then_logs_warning(self, mock_logger, monkeypatch):
         factory = PluginSystemManagerImplementationFactory(mock_logger)
         # Plugins are scanned lazily when first requested
         _ = factory.identifiers()
-        mock_logger.mock.log.assert_called_once_with(expected_msg, expected_severity)
+        mock_logger.mock.log.assert_called_once_with(expected_severity, expected_msg)
 
 
 class Test_PluginSystemManagerImplementationFactory_identifiers:

tests/python/openassetio/test_log.py

@@ -122,9 +122,9 @@ def test_only_messages_of_equal_or_greater_severity_are_relayed(
 
             for message_severity in all_severities:
                 mock_logger.mock.reset_mock()
-                severity_filter.log(msg, message_severity)
+                severity_filter.log(message_severity, msg)
                 if message_severity <= filter_severity:
                     mock_logger.mock.log.assert_called_once_with(
-                        msg, message_severity)
+                        message_severity, msg)
                 else:
                     mock_logger.mock.log.assert_not_called()