Repository: wntrblm/nox (python)

Issue:
Remove legacy support for running a function in run?
There's a legacy feature where `session.run([f, arg1], kw=1)` will run `f(arg1, kw=1)` for you, with a little bit of logging. I'm working on improving the static typing a bit, and this is messy. It's not in the static typing, it interferes with normal keyword arguments, and it has a bug: `env=` and `include_outer_env=` just get thrown away and aren't passed through. To support it properly, _everything_ would need to be `**kwargs`, which I'm trying to get rid of (better static typing, better docstrings, better signatures). I _could_ fix the static typing via TypedDict, but I'd much rather just remove the feature (described as "legacy" in comments) if it's acceptable.

By remove, I really mean remove the keyword argument passing, I don't care about the positional argument only form unless I have to add it to typing. We could leave that in, at least for a while, if someone wants it. It's the keywords that are problematic and currently buggy.

Pull request:
feat: use static arguments instead of **kwargs
Close #813. This provides better documentation, better signatures, and better static typing. This doesn't change too rapidly, so using `**` isn't that helpful. `**` is best for things that can't be determined statically.

A few kwargs that previously distinguished between not being passed and being passed now support `None`, which is simpler and also a little friendlier for callers.

> [!WARNING]
> This removes support for passing keyword arguments through to a function passed into run. `run(f, arg1, kw=kw)` is not supported anymore. This does keep support for `run(f, arg1)`. There's very little reason to ever do this, as it just runs the function for you with a smidge of logging. It's listed in the comment as a legacy feature. It was a little buggy before too, with `env=` for example being swallowed.


Patch:
diff --git a/nox/command.py b/nox/command.py
index 4882e3a..4f948e6 100644
--- a/nox/command.py
+++ b/nox/command.py
@@ -17,18 +17,23 @@ from __future__ import annotations
 import os
 import shlex
 import shutil
+import subprocess
 import sys
 from collections.abc import Iterable, Mapping, Sequence
-from typing import Any
 
 from nox.logger import logger
-from nox.popen import popen
+from nox.popen import DEFAULT_INTERRUPT_TIMEOUT, DEFAULT_TERMINATE_TIMEOUT, popen
 
 if sys.version_info < (3, 8):
     from typing_extensions import Literal
 else:
     from typing import Literal
 
+TYPE_CHECKING = False
+
+if TYPE_CHECKING:
+    from typing import IO
+
 ExternalType = Literal["error", True, False]
 
 
@@ -81,7 +86,10 @@ def run(
     success_codes: Iterable[int] | None = None,
     log: bool = True,
     external: ExternalType = False,
-    **popen_kws: Any,
+    stdout: int | IO[str] | None = None,
+    stderr: int | IO[str] = subprocess.STDOUT,
+    interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+    terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
 ) -> str | bool:
     """Run a command-line program."""
 
@@ -119,7 +127,13 @@ def run(
 
     try:
         return_code, output = popen(
-            [cmd_path, *str_args], silent=silent, env=env, **popen_kws
+            [cmd_path, *str_args],
+            silent=silent,
+            env=env,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
         )
 
         if return_code not in success_codes:
diff --git a/nox/popen.py b/nox/popen.py
index 2d37b55..f975146 100644
--- a/nox/popen.py
+++ b/nox/popen.py
@@ -21,6 +21,9 @@ import sys
 from collections.abc import Mapping, Sequence
 from typing import IO
 
+DEFAULT_INTERRUPT_TIMEOUT = 0.3
+DEFAULT_TERMINATE_TIMEOUT = 0.2
+
 
 def shutdown_process(
     proc: subprocess.Popen[bytes],
@@ -64,8 +67,8 @@ def popen(
     silent: bool = False,
     stdout: int | IO[str] | None = None,
     stderr: int | IO[str] = subprocess.STDOUT,
-    interrupt_timeout: float | None = 0.3,
-    terminate_timeout: float | None = 0.2,
+    interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+    terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
 ) -> tuple[int, str]:
     if silent and stdout is not None:
         raise ValueError(
diff --git a/nox/sessions.py b/nox/sessions.py
index b9d2669..756b09b 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -21,6 +21,7 @@ import hashlib
 import os
 import pathlib
 import re
+import subprocess
 import sys
 import unicodedata
 from collections.abc import (
@@ -41,9 +42,13 @@ import nox.command
 import nox.virtualenv
 from nox._decorators import Func
 from nox.logger import logger
+from nox.popen import DEFAULT_INTERRUPT_TIMEOUT, DEFAULT_TERMINATE_TIMEOUT
 from nox.virtualenv import UV, CondaEnv, PassthroughEnv, ProcessEnv, VirtualEnv
 
 if TYPE_CHECKING:
+    from typing import IO
+
+    from nox.command import ExternalType
     from nox.manifest import Manifest
 
 
@@ -269,13 +274,11 @@ class Session:
     cd = chdir
     """An alias for :meth:`chdir`."""
 
-    def _run_func(
-        self, func: Callable[..., Any], args: Iterable[Any], kwargs: Mapping[str, Any]
-    ) -> Any:
+    def _run_func(self, func: Callable[..., Any], args: Iterable[Any]) -> Any:
         """Legacy support for running a function through :func`run`."""
-        self.log(f"{func}(args={args!r}, kwargs={kwargs!r})")
+        self.log(f"{func}(args={args!r})")
         try:
-            return func(*args, **kwargs)
+            return func(*args)
         except Exception as e:
             logger.exception(f"Function {func!r} raised {e!r}.")
             raise nox.command.CommandFailed() from e
@@ -285,7 +288,14 @@ class Session:
         *args: str | os.PathLike[str],
         env: Mapping[str, str | None] | None = None,
         include_outer_env: bool = True,
-        **kwargs: Any,
+        silent: bool = False,
+        success_codes: Iterable[int] | None = None,
+        log: bool = True,
+        external: ExternalType | None = None,
+        stdout: int | IO[str] | None = None,
+        stderr: int | IO[str] = subprocess.STDOUT,
+        interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+        terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
     ) -> Any | None:
         """Run a command.
 
@@ -402,7 +412,14 @@ class Session:
             *args,
             env=env,
             include_outer_env=include_outer_env,
-            **kwargs,
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
+            external=external,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
         )
 
     def run_install(
@@ -410,7 +427,14 @@ class Session:
         *args: str | os.PathLike[str],
         env: Mapping[str, str | None] | None = None,
         include_outer_env: bool = True,
-        **kwargs: Any,
+        silent: bool = False,
+        success_codes: Iterable[int] | None = None,
+        log: bool = True,
+        external: ExternalType | None = None,
+        stdout: int | IO[str] | None = None,
+        stderr: int | IO[str] = subprocess.STDOUT,
+        interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+        terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
     ) -> Any | None:
         """Run a command in the install step.
 
@@ -470,7 +494,14 @@ class Session:
             *args,
             env=env,
             include_outer_env=include_outer_env,
-            **kwargs,
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
+            external=external,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
         )
 
     def run_always(
@@ -478,7 +509,14 @@ class Session:
         *args: str | os.PathLike[str],
         env: Mapping[str, str | None] | None = None,
         include_outer_env: bool = True,
-        **kwargs: Any,
+        silent: bool = False,
+        success_codes: Iterable[int] | None = None,
+        log: bool = True,
+        external: ExternalType | None = None,
+        stdout: int | IO[str] | None = None,
+        stderr: int | IO[str] = subprocess.STDOUT,
+        interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+        terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
     ) -> Any | None:
         """This is an alias to ``run_install``, which better describes the use case.
 
@@ -486,20 +524,37 @@ class Session:
         """
 
         return self.run_install(
-            *args, env=env, include_outer_env=include_outer_env, **kwargs
+            *args,
+            env=env,
+            include_outer_env=include_outer_env,
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
+            external=external,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
         )
 
     def _run(
         self,
         *args: str | os.PathLike[str],
         env: Mapping[str, str | None] | None = None,
-        include_outer_env: bool = True,
-        **kwargs: Any,
+        include_outer_env: bool,
+        silent: bool,
+        success_codes: Iterable[int] | None,
+        log: bool,
+        external: ExternalType | None,
+        stdout: int | IO[str] | None,
+        stderr: int | IO[str],
+        interrupt_timeout: float | None,
+        terminate_timeout: float | None,
     ) -> Any:
         """Like run(), except that it runs even if --install-only is provided."""
         # Legacy support - run a function given.
         if callable(args[0]):
-            return self._run_func(args[0], args[1:], kwargs)  # type: ignore[unreachable]
+            return self._run_func(args[0], args[1:])  # type: ignore[unreachable]
 
         # Combine the env argument with our virtualenv's env vars.
         if include_outer_env:
@@ -507,25 +562,48 @@ class Session:
             env = {**self.env, **overlay_env}
 
         # If --error-on-external-run is specified, error on external programs.
-        if self._runner.global_config.error_on_external_run:
-            kwargs.setdefault("external", "error")
+        if self._runner.global_config.error_on_external_run and external is None:
+            external = "error"
 
         # Allow all external programs when running outside a sandbox.
-        if not self.virtualenv.is_sandboxed:
-            kwargs["external"] = True
+        if (
+            not self.virtualenv.is_sandboxed
+            or args[0] in self.virtualenv.allowed_globals
+        ):
+            external = True
 
-        if args[0] in self.virtualenv.allowed_globals:
-            kwargs["external"] = True
+        if external is None:
+            external = False
 
         # Run a shell command.
-        return nox.command.run(args, env=env, paths=self.bin_paths, **kwargs)
+        return nox.command.run(
+            args,
+            env=env,
+            paths=self.bin_paths,
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
+            external=external,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
+        )
 
     def conda_install(
         self,
         *args: str,
         auto_offline: bool = True,
         channel: str | Sequence[str] = "",
-        **kwargs: Any,
+        env: Mapping[str, str] | None = None,
+        include_outer_env: bool = True,
+        silent: bool | None = None,
+        success_codes: Iterable[int] | None = None,
+        log: bool = True,
+        stdout: int | IO[str] | None = None,
+        stderr: int | IO[str] = subprocess.STDOUT,
+        interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+        terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
     ) -> None:
         """Install invokes `conda install`_ to install packages inside of the
         session's environment.
@@ -584,8 +662,8 @@ class Session:
         # Escape args that should be (conda-specific; pip install does not need this)
         args = _dblquote_pkg_install_args(args)
 
-        if "silent" not in kwargs:
-            kwargs["silent"] = True
+        if silent is None:
+            silent = True
 
         extraopts: list[str] = []
         if auto_offline and venv.is_offline():
@@ -608,11 +686,32 @@ class Session:
             *extraopts,
             *prefix_args,
             *args,
+            env=env,
+            include_outer_env=include_outer_env,
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
             external="error",
-            **kwargs,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
         )
 
-    def install(self, *args: str, **kwargs: Any) -> None:
+    def install(
+        self,
+        *args: str,
+        env: Mapping[str, str] | None = None,
+        include_outer_env: bool = True,
+        silent: bool | None = None,
+        success_codes: Iterable[int] | None = None,
+        log: bool = True,
+        external: ExternalType | None = None,
+        stdout: int | IO[str] | None = None,
+        stderr: int | IO[str] = subprocess.STDOUT,
+        interrupt_timeout: float | None = DEFAULT_INTERRUPT_TIMEOUT,
+        terminate_timeout: float | None = DEFAULT_TERMINATE_TIMEOUT,
+    ) -> None:
         """Install invokes `pip`_ to install packages inside of the session's
         virtualenv.
 
@@ -666,15 +765,27 @@ class Session:
         if self._runner.global_config.no_install and venv._reused:
             return
 
-        if "silent" not in kwargs:
-            kwargs["silent"] = True
+        if silent is None:
+            silent = True
 
         if isinstance(venv, VirtualEnv) and venv.venv_backend == "uv":
-            self._run(UV, "pip", "install", *args, external="error", **kwargs)
+            cmd = [UV, "pip", "install"]
         else:
-            self._run(
-                "python", "-m", "pip", "install", *args, external="error", **kwargs
-            )
+            cmd = ["python", "-m", "pip", "install"]
+        self._run(
+            *cmd,
+            *args,
+            env=env,
+            include_outer_env=include_outer_env,
+            external="error",
+            silent=silent,
+            success_codes=success_codes,
+            log=log,
+            stdout=stdout,
+            stderr=stderr,
+            interrupt_timeout=interrupt_timeout,
+            terminate_timeout=terminate_timeout,
+        )
 
     def notify(
         self,


Test patch:
diff --git a/tests/test_command.py b/tests/test_command.py
index 9a50093..ae398e9 100644
--- a/tests/test_command.py
+++ b/tests/test_command.py
@@ -195,7 +195,15 @@ def test_run_path_existent(tmp_path: Path):
     with mock.patch("nox.command.popen") as mock_command:
         mock_command.return_value = (0, "")
         nox.command.run([executable_name], silent=True, paths=[str(tmp_path)])
-        mock_command.assert_called_with([str(executable)], env=mock.ANY, silent=True)
+        mock_command.assert_called_with(
+            [str(executable)],
+            env=None,
+            silent=True,
+            stdout=None,
+            stderr=subprocess.STDOUT,
+            interrupt_timeout=0.3,
+            terminate_timeout=0.2,
+        )
 
 
 def test_run_external_warns(tmpdir, caplog):
diff --git a/tests/test_sessions.py b/tests/test_sessions.py
index 2a105b8..c5295f5 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -19,6 +19,7 @@ import logging
 import operator
 import os
 import shutil
+import subprocess
 import sys
 import tempfile
 from pathlib import Path
@@ -28,6 +29,7 @@ import pytest
 
 import nox.command
 import nox.manifest
+import nox.popen
 import nox.registry
 import nox.sessions
 import nox.virtualenv
@@ -38,6 +40,38 @@ HAS_CONDA = shutil.which("conda") is not None
 has_conda = pytest.mark.skipif(not HAS_CONDA, reason="Missing conda command.")
 
 
+def run_with_defaults(**kwargs):
+    return {
+        "env": None,
+        "silent": False,
+        "paths": None,
+        "success_codes": None,
+        "log": True,
+        "external": False,
+        "stdout": None,
+        "stderr": subprocess.STDOUT,
+        "interrupt_timeout": nox.popen.DEFAULT_INTERRUPT_TIMEOUT,
+        "terminate_timeout": nox.popen.DEFAULT_TERMINATE_TIMEOUT,
+        **kwargs,
+    }
+
+
+def _run_with_defaults(**kwargs):
+    return {
+        "env": None,
+        "include_outer_env": True,
+        "silent": False,
+        "success_codes": None,
+        "log": True,
+        "external": False,
+        "stdout": None,
+        "stderr": subprocess.STDOUT,
+        "interrupt_timeout": nox.popen.DEFAULT_INTERRUPT_TIMEOUT,
+        "terminate_timeout": nox.popen.DEFAULT_TERMINATE_TIMEOUT,
+        **kwargs,
+    }
+
+
 def test__normalize_path():
     envdir = "envdir"
     normalize = nox.sessions._normalize_path
@@ -250,10 +284,7 @@ class TestSession:
 
         run.assert_called_once_with(
             ("python", "-m", "pip", "install", "spam"),
-            env=mock.ANY,
-            external=mock.ANY,
-            paths=mock.ANY,
-            silent=mock.ANY,
+            **run_with_defaults(paths=mock.ANY, silent=True, env={}, external="error"),
         )
 
     def test_run_success(self):
@@ -323,9 +354,7 @@ class TestSession:
 
         run.assert_called_once_with(
             (sys.executable, "--version"),
-            external=True,
-            env=mock.ANY,
-            paths=None,
+            **run_with_defaults(external=True, env=mock.ANY),
         )
 
     def test_run_external_condaenv(self):
@@ -342,9 +371,9 @@ class TestSession:
 
         run.assert_called_once_with(
             ("conda", "--version"),
-            external=True,
-            env=mock.ANY,
-            paths=["/path/to/env/bin"],
+            **run_with_defaults(
+                external=True, env=mock.ANY, paths=["/path/to/env/bin"]
+            ),
         )
 
     def test_run_external_with_error_on_external_run(self):
@@ -552,8 +581,7 @@ class TestSession:
                 "/path/to/conda/env",
                 "requests",
                 "urllib3",
-                silent=True,
-                external="error",
+                **_run_with_defaults(silent=True, external="error"),
             )
 
     @pytest.mark.parametrize(
@@ -627,8 +655,7 @@ class TestSession:
                 "requests",
                 # this will be double quoted if unquoted constraint is present
                 passed_arg,
-                silent=False,
-                external="error",
+                **_run_with_defaults(silent=False, external="error"),
             )
 
     def test_install_bad_args_no_arg(self):
@@ -673,8 +700,7 @@ class TestSession:
                 "install",
                 "requests",
                 "urllib3",
-                silent=True,
-                external="error",
+                **_run_with_defaults(silent=True, external="error"),
             )
 
     def test_install_non_default_kwargs(self):
@@ -703,8 +729,7 @@ class TestSession:
                 "install",
                 "requests",
                 "urllib3",
-                silent=False,
-                external="error",
+                **_run_with_defaults(silent=False, external="error"),
             )
 
     def test_install_no_venv_failure(self):
@@ -849,8 +874,7 @@ class TestSession:
                 "install",
                 "requests",
                 "urllib3",
-                silent=False,
-                external="error",
+                **_run_with_defaults(silent=False, external="error"),
             )
 
     def test___slots__(self):