Repository: wntrblm/nox
Pull request #649 (2022-09-04 07:47:49)

fix: nox.session.run-ing commands with pathlib.Path arguments
Everything previously really worked fine in theory, but the debug string
which is logged (and calls shlex.quote) was blowing up trying
to quote a pathlib.Path object. I'm somewhat surprised upstream
shlex.quote doesn't support pathlib.Path objects (yet?) but for
now this just falls back to a cruder representation.

(Obviously there are other ways to handle this, like calling str() on all the args, let me know if you prefer something else).

This also apparently doesn't work on Windows, but that's because `subprocess.Popen` is apparently broken with WindowsPaths. The upstream bug is  https://github.com/python/cpython/issues/85815.

henryiii: I'd think `os.fspath(x) if isinstance(x, os.PathLike) for x in args` would be the right way to handle it (without introducing bugs if something non-pathlike gets passed in that has a str representation - which everything in Python does).
Julian: Happy to do that instead obviously -- do you mean just what I just pushed (I don't think one needs to do the isinstance check, unless you meant to filter out non-fspath args silently, which seems unlikely)?
henryiii: Actually, I guess it doesn't need a check, since `os.fspath` passes through `str` / `bytes` and only works on `os.PathLike` otherwise. It just seems a little odd to pass a non-path into `os.fspath`, and there will be non-paths coming in. It also might be a bit more extendable - if any other object protocols were ever added, you'd then need the check.
henryiii: FYI, I recently wrote a class that wrapped a commandline tool, and realized I could define `__fspath__()` instead of making up an arbitrary attribute holding the path to the wrapped exe. Things like this would cause it to just work as if it was the wrapped exe (this is exactly what I did with my own run function). Even without it, `os.fspath(thing)` is easier to remember than `thing.some_name_I_made_up`.

But the important thing is the `str` is different than `__fspath__()`, and the fspath one is the correct one to use.
Julian: Right I've made CLIs that worked precisely as you describe (where they allow lots of fun objects to become filepaths). Certainly supporting *more* objects would indeed be nice.

Patch:
diff --git a/nox/command.py b/nox/command.py
index 8a55c1f..9b828b4 100644
--- a/nox/command.py
+++ b/nox/command.py
@@ -71,8 +71,7 @@ def _clean_env(env: dict[str, str] | None) -> dict[str, str] | None:
 
 
 def _shlex_join(args: Sequence[str]) -> str:
-    # shlex.join() was added in Python 3.8
-    return " ".join(shlex.quote(arg) for arg in args)
+    return " ".join(shlex.quote(os.fspath(arg)) for arg in args)
 
 
 def run(
diff --git a/nox/sessions.py b/nox/sessions.py
index bae3074..bd42bc7 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -305,6 +305,16 @@ class Session:
                # Display coverage report even when tests fail.
                session.run("coverage", "report")
 
+        If you pass ``silent=True``, you can capture the output of a command that would
+        otherwise be shown to the user. For example to get the current Git commit ID::
+
+            out = session.run(
+                "git", "rev-parse", "--short", "HEAD",
+                external=True, silent=True
+            )
+
+            print("Current Git commit is", out.strip())
+
         :param env: A dictionary of environment variables to expose to the
             command. By default, all environment variables are passed.
         :type env: dict or None
@@ -330,6 +340,12 @@ class Session:
             them. Set to ``None`` to never send a kill signal.
             Default: ``0.2``
         :type terminate_timeout: float or None
+        :param stdout: Redirect standard output of the command into a file. Can't be
+            combined with *silent*.
+        :type stdout: file or file descriptor
+        :param stderr: Redirect standard output of the command into a file. Can't be
+            combined with *silent*.
+        :type stderr: file or file descriptor
         """
         if not args:
             raise ValueError("At least one argument required to run().")


Test patch:
diff --git a/tests/test_command.py b/tests/test_command.py
index a05ab82..efda160 100644
--- a/tests/test_command.py
+++ b/tests/test_command.py
@@ -22,6 +22,7 @@ import signal
 import subprocess
 import sys
 import time
+from pathlib import Path
 from textwrap import dedent
 from unittest import mock
 
@@ -112,6 +113,19 @@ def test_run_verbosity_failed_command(capsys, caplog):
     assert not logs
 
 
+@pytest.mark.skipif(
+    platform.system() == "Windows",
+    reason="See https://github.com/python/cpython/issues/85815",
+)
+def test_run_non_str():
+    result = nox.command.run(
+        [Path(PYTHON), "-c", "import sys; print(sys.argv)", Path(PYTHON)],
+        silent=True,
+    )
+
+    assert PYTHON in result
+
+
 def test_run_env_unicode():
     result = nox.command.run(
         [PYTHON, "-c", 'import os; print(os.environ["SIGIL"])'],