-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve echo_via_pager behaviour in face of errors #2775
base: main
Are you sure you want to change the base?
Conversation
b715ba2
to
ce88a00
Compare
I've had a look at #350 and the new behaviour sounds like what @amjith describes in the comment #350 (comment) The fix by @untitaker is still in place (https:/pallets/click/commits/e71f0a040a81e03276b25ab1d1b0671822ab331b) – now in the finally block, which should be even better. For me this works really well in all the situations I was able to come up with so far. |
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why OSError
has been captured here; I think it's probably better to remove it and re-introduce it with a comment or a more specific solution if it causes a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSError
used to be IOError
and it's in since the beginning (aef1650).
If write
fails for some reason, I'd like to see an error message, rather than swallowing any error that might occur – but I'm happy to revert this change if the reviewer disagrees or there is a reason I'm not aware of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most likely this is done for less noisy behavior in broken pipes or broken SSH sessions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, now I also ran into a case. Pushed a version now that ignores BrokenPipeError
specifically
…cho in the generator This commit fixes pallets#2674 (echo_via_pager with generators leaves terminal in broken state). When running the program to reproduce the issue (thanks to @0xDEC0DE for providing it in the issue mentioned above!) ``` import click def output_generator(): counter = 0 while True: yield "this is a line of output\n" if counter == 1024: click.echo("kaboom", err=True) click.get_current_context().exit(0) counter += 1 @click.command def kaboom(): click.echo_via_pager(output_generator) if __name__ == "__main__": kaboom() ``` The "kaboom" message will now be visible immediately (the error will cause click to terminate the pager). ZSH and bash will not be in a broken state (tested using bash and zsh on macos). Further changes: - Error handling: terminate the pager and close the file descriptors in a finally block - KeyboardInterrupt: the pager code 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. - tests: improved the echo_via_pager tests by using a named tuple and separate assertions for pager output, stderr and stdout. Added additional test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have rights to approve, but this looks like it will do the trick.
fixes #2674 (echo_via_pager with generators leaves terminal in broken state).
should fail without the change.
.. versionchanged::
entries in any relevant code docs.