Deprecate unnamed arguments to the run() function (#62)

Author: Anthchirp

HISTORY.rst

@@ -2,6 +2,13 @@
 History
 =======
 
+2.2.0
+-----
+* Calling the run() function with unnamed arguments (other than the command
+  list as the first argument) is now deprecated. As a number of arguments
+  will be removed in a future version the use of unnamed arguments will
+  cause future confusion. `Use explicit keyword arguments instead (#62). <https://github.com/DiamondLightSource/python-procrunner/pull/62>`_
+
 2.1.0 (2020-09-05)
 ------------------
 * `Deprecated array access on the return object (#60). <https://github.com/DiamondLightSource/python-procrunner/pull/60>`_

procrunner/__init__.py

@@ -1,4 +1,5 @@
 import codecs
+import functools
 import io
 import logging
 import os
@@ -413,6 +414,22 @@ def update(self, dictionary):
         self._extras.update(dictionary)
 
 
+def _deprecate_argument_calling(f):
+    @functools.wraps(f)
+    def wrapper(*args, **kwargs):
+        if len(args) > 1:
+            warnings.warn(
+                "Calling procrunner.run() with unnamed arguments (apart from "
+                "the command) is deprecated. Use keyword arguments instead.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+        return f(*args, **kwargs)
+
+    return wrapper
+
+
+@_deprecate_argument_calling
 def run(
     command,
     timeout=None,

tests/test_procrunner.py

@@ -21,7 +21,7 @@ def test_run_command_aborts_after_timeout_legacy(
 
     with pytest.raises(RuntimeError):
         with pytest.warns(DeprecationWarning, match="timeout"):
-            procrunner.run(task, -1, False)
+            procrunner.run(task, timeout=-1, debug=False)
 
     assert mock_subprocess.Popen.called
     assert mock_process.terminate.called
@@ -42,7 +42,7 @@ def test_run_command_aborts_after_timeout(
     task = ["___"]
 
     with pytest.raises(RuntimeError):
-        procrunner.run(task, -1, False, raise_timeout_exception=True)
+        procrunner.run(task, timeout=-1, debug=False, raise_timeout_exception=True)
 
     assert mock_subprocess.Popen.called
     assert mock_process.terminate.called
@@ -83,8 +83,8 @@ def streamreader_processing(*args, **kwargs):
 
     actual = procrunner.run(
         command,
-        0.5,
-        False,
+        timeout=0.5,
+        debug=False,
         callback_stdout=mock.sentinel.callback_stdout,
         callback_stderr=mock.sentinel.callback_stderr,
         working_directory=mock.sentinel.cwd,
@@ -128,7 +128,9 @@ def streamreader_processing(*args, **kwargs):
 def test_default_process_environment_is_parent_environment(mock_subprocess):
     mock_subprocess.Popen.side_effect = NotImplementedError()  # cut calls short
     with pytest.raises(NotImplementedError):
-        procrunner.run([mock.Mock()], -1, False, raise_timeout_exception=True)
+        procrunner.run(
+            [mock.Mock()], timeout=-1, debug=False, raise_timeout_exception=True
+        )
     assert mock_subprocess.Popen.call_args[1]["env"] == os.environ
 
 
@@ -140,8 +142,8 @@ def test_pass_custom_environment_to_process(mock_subprocess):
     with pytest.raises(NotImplementedError):
         procrunner.run(
             [mock.Mock()],
-            -1,
-            False,
+            timeout=-1,
+            debug=False,
             environment=copy.copy(mock_env),
             raise_timeout_exception=True,
         )
@@ -157,8 +159,8 @@ def test_pass_custom_environment_to_process_and_add_another_value(mock_subproces
     with pytest.raises(NotImplementedError):
         procrunner.run(
             [mock.Mock()],
-            -1,
-            False,
+            timeout=-1,
+            debug=False,
             environment=copy.copy(mock_env1),
             environment_override=copy.copy(mock_env2),
             raise_timeout_exception=True,
@@ -175,8 +177,8 @@ def test_use_default_process_environment_and_add_another_value(mock_subprocess):
     with pytest.raises(NotImplementedError):
         procrunner.run(
             [mock.Mock()],
-            -1,
-            False,
+            timeout=-1,
+            debug=False,
             environment_override=copy.copy(mock_env2),
             raise_timeout_exception=True,
         )
@@ -204,8 +206,8 @@ def test_use_default_process_environment_and_override_a_value(mock_subprocess):
     with pytest.raises(NotImplementedError):
         procrunner.run(
             [mock.Mock()],
-            -1,
-            False,
+            timeout=-1,
+            debug=False,
             environment_override={
                 random_environment_variable: "X" + random_environment_value
             },

tests/test_procrunner_system.py

@@ -118,3 +118,14 @@ def test_timeout_behaviour(tmp_path):
     assert te.value.stderr == b""
     assert te.value.timeout == 0.1
     assert te.value.cmd == command
+
+
+def test_argument_deprecation(tmp_path):
+    with pytest.warns(DeprecationWarning, match="keyword arguments"):
+        result = procrunner.run(
+            [sys.executable, "-V"],
+            None,
+            working_directory=tmp_path,
+        )
+    assert not result.returncode
+    assert result.stderr or result.stdout