RFC: respect $PAGER env vars, make -FRSX default in less (#5906)

dvc/utils/pager.py

@@ -14,40 +14,80 @@ logger = logging.getLogger(__name__)
 
 
 DEFAULT_PAGER = "less"
-DEFAULT_PAGER_FORMATTED = (
-    f"{DEFAULT_PAGER} --chop-long-lines --clear-screen --RAW-CONTROL-CHARS"
-)
+LESS = "LESS"
+PAGER_ENV = "PAGER"
 
 
-def make_pager(cmd):
+def prepare_default_pager(
+    clear_screen: bool = False,
+    quit_if_one_screen: bool = True,
+    ansi_escapes: bool = True,
+    chop_long_lines: bool = True,
+    no_init: bool = True,
+    no_tilde: bool = False,
+) -> str:
+    args = [DEFAULT_PAGER]
+    if clear_screen:
+        args.append("--clear-screen")  # -c
+    if quit_if_one_screen:
+        args.append("--quit-if-one-screen")  # -F
+    if ansi_escapes:
+        args.append("--RAW-CONTROL-CHARS")  # -R
+    if chop_long_lines:
+        args.append("--chop-long-lines")  # -S
+    if no_init:
+        args.append("--no-init")  # -X
+    if no_tilde:
+        args.append("--tilde")  # -~
+
+    return " ".join(args)
+
+
+def make_pager(cmd=None):
     def _pager(text):
         return pydoc.tempfilepager(pydoc.plain(text), cmd)
 
-    return _pager
+    return _pager if cmd else pydoc.plainpager
 
 
 def find_pager():
     if not sys.stdout.isatty():
-        return pydoc.plainpager
+        return None
 
-    env_pager = os.getenv(DVC_PAGER)
-    if env_pager:
-        return make_pager(env_pager)
+    # pylint: disable=redefined-outer-name
+    pager = os.getenv(DVC_PAGER)
+    if not pager:
+        pager = os.getenv(PAGER_ENV)
+    if not pager:
+        if os.system(f"({DEFAULT_PAGER}) 2>{os.devnull}") != 0:
+            logger.warning(
+                "Unable to find `less` in the PATH. Check out "
+                "{} for more info.".format(
+                    format_link("https://man.dvc.org/pipeline/show")
+                )
+            )
+        else:
+            pager = DEFAULT_PAGER
 
-    if os.system(f"({DEFAULT_PAGER}) 2>{os.devnull}") == 0:
-        return make_pager(DEFAULT_PAGER_FORMATTED)
-
-    logger.warning(
-        "Unable to find `less` in the PATH. Check out "
-        "{} for more info.".format(
-            format_link("https://man.dvc.org/pipeline/show")
+    if pager == DEFAULT_PAGER:
+        # if pager is less (i.e. default), regardless of `$LESS`, apply `-RS`.
+        # `-R` is required to render ansi escape sequences for exp show
+        # and, `-S` is required for horizontal scrolling.
+        less_env = bool(os.getenv(LESS))
+        pager = prepare_default_pager(
+            ansi_escapes=True,
+            chop_long_lines=True,
+            quit_if_one_screen=not less_env,
+            no_init=not less_env,
         )
-    )
-    return pydoc.plainpager
+
+    return pager
 
 
 def pager(text: str) -> None:
-    find_pager()(text)
+    _pager = find_pager()
+    logger.trace(f"Using pager: '{_pager}'")  # type: ignore[attr-defined]
+    make_pager(_pager)(text)
 
 
 class DvcPager(Pager):

tests/unit/ui/test_pager.py

@@ -0,0 +1,99 @@
+import pytest
+
+from dvc.env import DVC_PAGER
+from dvc.utils.pager import (
+    DEFAULT_PAGER,
+    LESS,
+    PAGER_ENV,
+    find_pager,
+    make_pager,
+    pager,
+)
+
+
+@pytest.fixture(autouse=True)
+def clear_envs(monkeypatch):
+    monkeypatch.delenv(DVC_PAGER, raising=False)
+    monkeypatch.delenv(PAGER_ENV, raising=False)
+    monkeypatch.delenv(LESS, raising=False)
+
+
+def test_find_pager_when_not_isatty(mocker):
+    mocker.patch("sys.stdout.isatty", return_value=False)
+    assert find_pager() is None
+
+
+def test_find_pager_uses_custom_pager_when_dvc_pager_env_var_is_defined(
+    mocker, monkeypatch
+):
+    monkeypatch.setenv(DVC_PAGER, "my-pager")
+    mocker.patch("sys.stdout.isatty", return_value=True)
+
+    assert find_pager() == "my-pager"
+
+
+def test_find_pager_uses_custom_pager_when_pager_env_is_defined(
+    mocker, monkeypatch
+):
+    monkeypatch.setenv(PAGER_ENV, "my-pager")
+    mocker.patch("sys.stdout.isatty", return_value=True)
+
+    assert find_pager() == "my-pager"
+
+
+def test_find_pager_uses_default_pager_when_found(mocker):
+    mocker.patch("sys.stdout.isatty", return_value=True)
+    mocker.patch("os.system", return_value=0)
+
+    assert DEFAULT_PAGER in find_pager()
+
+
+def test_find_pager_fails_to_find_any_pager(mocker):
+    mocker.patch("os.system", return_value=1)
+    mocker.patch("sys.stdout.isatty", return_value=True)
+
+    assert find_pager() is None
+
+
+@pytest.mark.parametrize("env", [DVC_PAGER, PAGER_ENV, None])
+def test_dvc_sets_default_options_on_less_without_less_env(
+    mocker, monkeypatch, env
+):
+    if env:
+        monkeypatch.setenv(env, "less")
+    mocker.patch("sys.stdout.isatty", return_value=True)
+    mocker.patch("os.system", return_value=0)
+
+    assert find_pager() == (
+        "less --quit-if-one-screen --RAW-CONTROL-CHARS"
+        " --chop-long-lines --no-init"
+    )
+
+
+@pytest.mark.parametrize("env", [DVC_PAGER, PAGER_ENV, None])
+def test_dvc_sets_some_options_on_less_if_less_env_defined(
+    mocker, monkeypatch, env
+):
+    if env:
+        monkeypatch.setenv(env, "less")
+    mocker.patch("sys.stdout.isatty", return_value=True)
+    mocker.patch("os.system", return_value=0)
+    monkeypatch.setenv(LESS, "-R")
+
+    assert find_pager() == "less --RAW-CONTROL-CHARS --chop-long-lines"
+
+
+def test_make_pager_when_no_pager_found(mocker, monkeypatch):
+    assert make_pager(None).__name__ == "plainpager"
+
+
+def test_pager(mocker, monkeypatch):
+    monkeypatch.setenv(DVC_PAGER, "my-pager")
+    mocker.patch("sys.stdout.isatty", return_value=True)
+
+    m_make_pager = mocker.patch("dvc.utils.pager.make_pager")
+    _pager = m_make_pager.return_value = mocker.MagicMock()
+
+    pager("hello world")
+    m_make_pager.assert_called_once_with("my-pager")
+    _pager.assert_called_once_with("hello world")