diff --git a/CHANGES.rst b/CHANGES.rst index b5d970117..5962e3d66 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -31,6 +31,16 @@ Unreleased - When generating a command's name from a decorated function's name, the suffixes ``_command``, ``_cmd``, ``_group``, and ``_grp`` are removed. :issue:`2322` +- Improve ``echo_via_pager`` behaviour in face of errors. + :issue:`2674` + + - Terminate the pager in case a generator passed to ``echo_via_pager`` + raises an exception. + - Ensure to always close the pipe to the pager process and wait for it + to terminate. + - ``echo_via_pager`` will not ignore ``KeyboardInterrupt`` anymore. This + allows the user to search for future output of the generator when + using less and then aborting the program using ctrl-c. Version 8.1.8 diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index 923641a44..a3af250c3 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -424,26 +424,33 @@ def _pipepager(generator: cabc.Iterable[str], cmd: str, color: bool | None) -> N text = strip_ansi(text) stdin.write(text.encode(encoding, "replace")) - except (OSError, KeyboardInterrupt): - pass - else: + stdin.flush() + except Exception as e: + # In case there is an exception we want to close the pager immediately + # and let the caller handle it. + # Otherwise the pager will keep running, and the user may not notice + # the error message, or worse yet it may leave the terminal in a broken state. + c.terminate() + raise e + finally: + # We must close stdin and wait for the pager to exit before we continue stdin.close() - # Less doesn't respect ^C, but catches it for its own UI purposes (aborting - # search or other commands inside less). - # - # That means when the user hits ^C, the parent process (click) terminates, - # but less is still alive, paging the output and messing up the terminal. - # - # If the user wants to make the pager exit on ^C, they should set - # `LESS='-K'`. It's not our decision to make. - while True: - try: - c.wait() - except KeyboardInterrupt: - pass - else: - break + # Less doesn't respect ^C, but catches it for its own UI purposes (aborting + # search or other commands inside less). + # + # That means when the user hits ^C, the parent process (click) terminates, + # but less is still alive, paging the output and messing up the terminal. + # + # If the user wants to make the pager exit on ^C, they should set + # `LESS='-K'`. It's not our decision to make. + while True: + try: + c.wait() + except KeyboardInterrupt: + pass + else: + break def _tempfilepager(generator: cabc.Iterable[str], cmd: str, color: bool | None) -> None: diff --git a/tests/test_utils.py b/tests/test_utils.py index b8b65e295..a146fe99a 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -2,7 +2,9 @@ import pathlib import stat import sys +from collections import namedtuple from io import StringIO +from tempfile import tempdir import pytest @@ -181,32 +183,172 @@ def _test_gen_func(): yield "abc" +def _test_gen_func_fails(): + yield "test" + raise RuntimeError("This is a test.") + + +def _test_gen_func_echo(file=None): + yield "test" + click.echo("hello", file=file) + yield "test" + + +def _test_simulate_keyboard_interrupt(file=None): + yield "output_before_keyboard_interrupt" + raise KeyboardInterrupt() + + +EchoViaPagerTest = namedtuple( + "EchoViaPagerTest", + ( + "description", + "test_input", + "expected_pager", + "expected_stdout", + "expected_stderr", + "expected_error", + ), +) + + @pytest.mark.skipif(WIN, reason="Different behavior on windows.") -@pytest.mark.parametrize("cat", ["cat", "cat ", "cat "]) @pytest.mark.parametrize( "test", [ - # We need lambda here, because pytest will - # reuse the parameters, and then the generators - # are already used and will not yield anymore - ("just text\n", lambda: "just text"), - ("iterable\n", lambda: ["itera", "ble"]), - ("abcabc\n", lambda: _test_gen_func), - ("abcabc\n", lambda: _test_gen_func()), - ("012345\n", lambda: (c for c in range(6))), + # We need to pass a parameter function instead of a plain param + # as pytest.mark.parametrize will reuse the parameters causing the + # generators to be used up so they will not yield anymore + EchoViaPagerTest( + description="Plain string argument", + test_input=lambda: "just text", + expected_pager="just text\n", + expected_stdout="", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="Iterable argument", + test_input=lambda: ["itera", "ble"], + expected_pager="iterable\n", + expected_stdout="", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="Generator function argument", + test_input=lambda: _test_gen_func, + expected_pager="abcabc\n", + expected_stdout="", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="String generator argument", + test_input=lambda: _test_gen_func(), + expected_pager="abcabc\n", + expected_stdout="", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="Number generator expression argument", + test_input=lambda: (c for c in range(6)), + expected_pager="012345\n", + expected_stdout="", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="Exception in generator function argument", + test_input=lambda: _test_gen_func_fails, + # Because generator throws early on, the pager did not have + # a chance yet to write the file. + expected_pager=None, + expected_stdout="", + expected_stderr="", + expected_error=RuntimeError, + ), + EchoViaPagerTest( + description="Exception in generator argument", + test_input=lambda: _test_gen_func_fails, + # Because generator throws early on, the pager did not have a + # chance yet to write the file. + expected_pager=None, + expected_stdout="", + expected_stderr="", + expected_error=RuntimeError, + ), + EchoViaPagerTest( + description="Keyboard interrupt should not terminate the pager", + test_input=lambda: _test_simulate_keyboard_interrupt(), + # Due to the keyboard interrupt during pager execution, click program + # should abort, but the pager should stay open. + # This allows users to cancel the program and search in the pager + # output, before they decide to terminate the pager. + expected_pager="output_before_keyboard_interrupt", + expected_stdout="", + expected_stderr="", + expected_error=KeyboardInterrupt, + ), + EchoViaPagerTest( + description="Writing to stdout during generator execution", + test_input=lambda: _test_gen_func_echo(), + expected_pager="testtest\n", + expected_stdout="hello\n", + expected_stderr="", + expected_error=None, + ), + EchoViaPagerTest( + description="Writing to stderr during generator execution", + test_input=lambda: _test_gen_func_echo(file=sys.stderr), + expected_pager="testtest\n", + expected_stdout="", + expected_stderr="hello\n", + expected_error=None, + ), ], ) -def test_echo_via_pager(monkeypatch, capfd, cat, test): - monkeypatch.setitem(os.environ, "PAGER", cat) +def test_echo_via_pager(monkeypatch, capfd, test): + pager_out_tmp = f"{tempdir}/pager_out.txt" + + if os.path.exists(pager_out_tmp): + os.remove(pager_out_tmp) + + monkeypatch.setitem(os.environ, "PAGER", f"cat > {pager_out_tmp}") monkeypatch.setattr(click._termui_impl, "isatty", lambda x: True) - expected_output = test[0] - test_input = test[1]() + test_input = test.test_input() + expected_pager = test.expected_pager + expected_stdout = test.expected_stdout + expected_stderr = test.expected_stderr + expected_error = test.expected_error - click.echo_via_pager(test_input) + if expected_error: + with pytest.raises(expected_error): + click.echo_via_pager(test_input) + else: + click.echo_via_pager(test_input) out, err = capfd.readouterr() - assert out == expected_output + + if os.path.exists(pager_out_tmp): + with open(pager_out_tmp) as f: + pager = f.read() + else: + # The pager process was not started or has been + # terminated before it could finish writing + pager = None + + assert ( + pager == expected_pager + ), f"Unexpected pager output in test case '{test.description}'" + assert ( + out == expected_stdout + ), f"Unexpected stdout in test case '{test.description}'" + assert ( + err == expected_stderr + ), f"Unexpected stderr in test case '{test.description}'" @pytest.mark.skipif(WIN, reason="Test does not make sense on Windows.")