Repository navigation
gh-122622: Add PyREPL simple_interact.statement_submitted_hook #157241
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
base: main
Are you sure you want to change the base?
Changes from all commits
4c18d3b
b988e6f
7d9ddbf
ad17396
0ef602c
8ede9b2
6d7dfe0
d45c1d3
e1186d5
f615af2
f766b48
a81c71a
a98d37a
d948c28
ce55702
8665769
b7e9986
753ed83
d2a7a45
ac8d177
e28071a
d6cd848
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,11 +3,12 @@ | |||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||
| import unittest | ||||||||||||||||||||||||
| from unittest.mock import ANY, patch | ||||||||||||||||||||||||
| from unittest.mock import ANY, MagicMock, patch | ||||||||||||||||||||||||
| from textwrap import dedent | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| from test.support import force_not_colorized | ||||||||||||||||||||||||
| from test.support import force_colorized, force_not_colorized | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| from _pyrepl import simple_interact | ||||||||||||||||||||||||
| from _pyrepl.console import InteractiveColoredConsole | ||||||||||||||||||||||||
| from _pyrepl.simple_interact import ( | ||||||||||||||||||||||||
| _more_lines, | ||||||||||||||||||||||||
|
|
@@ -358,3 +359,150 @@ def f(): | |||||||||||||||||||||||
| count = sum("'return' in a 'finally' block" in str(w.message) | ||||||||||||||||||||||||
| for w in caught) | ||||||||||||||||||||||||
| self.assertEqual(count, 1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| class TestStatementSubmittedHook(unittest.TestCase): | ||||||||||||||||||||||||
| def _run_interactive(self, statements, hook): | ||||||||||||||||||||||||
| console = InteractiveColoredConsole() | ||||||||||||||||||||||||
| statement_iter = iter(statements) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def fake_multiline_input(more_lines, ps1, ps2): | ||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||
| return next(statement_iter) | ||||||||||||||||||||||||
| except StopIteration: | ||||||||||||||||||||||||
| raise EOFError | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| output = io.StringIO() | ||||||||||||||||||||||||
| with ( | ||||||||||||||||||||||||
| patch.object(simple_interact, "statement_submitted_hook", hook), | ||||||||||||||||||||||||
| patch.object( | ||||||||||||||||||||||||
| simple_interact, | ||||||||||||||||||||||||
| "multiline_input", | ||||||||||||||||||||||||
| side_effect=fake_multiline_input, | ||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||
| patch.object(simple_interact, "_get_reader"), | ||||||||||||||||||||||||
| patch.object(simple_interact, "append_history_file"), | ||||||||||||||||||||||||
| patch("_pyrepl.readline._setup"), | ||||||||||||||||||||||||
| contextlib.redirect_stdout(output), | ||||||||||||||||||||||||
| contextlib.redirect_stderr(output), | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't rely on mocks of internal attributes. I'd test this with a PTY instead, maybe in Example: cpython/Lib/test/test_pyrepl/test_pyrepl.py Lines 2482 to 2492 in d948c28
(This applies to all tests in this PR, I'd try to fit all of them into a few tests in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A PTY test makes sense here. The unit tests still exercise real statement execution, so I see no need to replace all of them. One detail: the example needs
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added e28071a0a0a: Add PTY test. The new TestMain test installs the hook through actual REPL input and checks that labelled hook errors reach the terminal while subsequent statements still execute. I retained the focused unit tests, following Pablo's suggestion. The inline escape-sequence example has also been removed in b7e9986fc46: Simplify comment to remove example; this PTY test does not claim to validate buffering behavior. Validation: |
||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||
| simple_interact.run_multiline_interactive_console(console) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return output.getvalue(), console.locals | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_called_before_statement_execution(self): | ||||||||||||||||||||||||
| statement = "if True:\n print('statement executed')\n" | ||||||||||||||||||||||||
| escape_sequence = "\x1b]633;C\x07" | ||||||||||||||||||||||||
| hook = MagicMock( | ||||||||||||||||||||||||
| side_effect=lambda statement: print(escape_sequence, end="") | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| output, _ = self._run_interactive([statement], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| hook.assert_called_once_with(statement) | ||||||||||||||||||||||||
| self.assertEqual(output, f"{escape_sequence}statement executed\n") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_receives_input_without_final_auto_indent(self): | ||||||||||||||||||||||||
| statement = "if True:\n x = 1\n " | ||||||||||||||||||||||||
| hook = MagicMock() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| _, namespace = self._run_interactive([statement], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| hook.assert_called_once_with("if True:\n x = 1\n") | ||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @force_not_colorized | ||||||||||||||||||||||||
| def test_hook_exception_is_displayed(self): | ||||||||||||||||||||||||
| hook = MagicMock(side_effect=RuntimeError("hook error")) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| output, namespace = self._run_interactive(["x = 1"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertIn( | ||||||||||||||||||||||||
| "Error in statement_submitted_hook:\n" | ||||||||||||||||||||||||
| "Traceback (most recent call last):\n", | ||||||||||||||||||||||||
| output, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| self.assertIn("statement_submitted_hook(", output) | ||||||||||||||||||||||||
| self.assertIn("RuntimeError: hook error", output) | ||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @force_colorized | ||||||||||||||||||||||||
| def test_hook_exception_is_colorized(self): | ||||||||||||||||||||||||
| def hook(statement): | ||||||||||||||||||||||||
| raise RuntimeError("hook error") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| output, namespace = self._run_interactive(["x = 1"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertIn("Error in statement_submitted_hook:\n", output) | ||||||||||||||||||||||||
| self.assertRegex(output, r"\x1b\[[0-9;]+m") | ||||||||||||||||||||||||
| self.assertIn("hook error", output) | ||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @force_not_colorized | ||||||||||||||||||||||||
| def test_hook_exception_preserves_last_exception(self): | ||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||
| raise ValueError("previous user error") | ||||||||||||||||||||||||
| except ValueError as exc: | ||||||||||||||||||||||||
| previous_exception = exc | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| last_attributes = { | ||||||||||||||||||||||||
| "last_exc": previous_exception, | ||||||||||||||||||||||||
| "last_type": type(previous_exception), | ||||||||||||||||||||||||
| "last_value": previous_exception, | ||||||||||||||||||||||||
| "last_traceback": previous_exception.__traceback__, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| for initialized in (True, False): | ||||||||||||||||||||||||
| with self.subTest(initialized=initialized), contextlib.ExitStack() as stack: | ||||||||||||||||||||||||
| for name, value in last_attributes.items(): | ||||||||||||||||||||||||
| stack.enter_context(patch.object(sys, name, value, create=True)) | ||||||||||||||||||||||||
| if not initialized: | ||||||||||||||||||||||||
| delattr(sys, name) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| hook = MagicMock(side_effect=RuntimeError("hook error")) | ||||||||||||||||||||||||
| output, namespace = self._run_interactive(["x = 1"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertIn("RuntimeError: hook error", output) | ||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
| for name, value in last_attributes.items(): | ||||||||||||||||||||||||
| if initialized: | ||||||||||||||||||||||||
| self.assertIs(getattr(sys, name), value) | ||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||
| self.assertFalse(hasattr(sys, name)) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_keyboard_interrupt_cancels_submission(self): | ||||||||||||||||||||||||
| hook = MagicMock(side_effect=[KeyboardInterrupt, None]) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| output, namespace = self._run_interactive(["x = 1", "y = 2"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertIn("\nKeyboardInterrupt\n", output) | ||||||||||||||||||||||||
| self.assertNotIn("Error in statement_submitted_hook:", output) | ||||||||||||||||||||||||
| self.assertNotIn("x", namespace) | ||||||||||||||||||||||||
| self.assertEqual(namespace["y"], 2) | ||||||||||||||||||||||||
| self.assertEqual(hook.call_count, 2) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_system_exit_exits_repl(self): | ||||||||||||||||||||||||
| hook = MagicMock(side_effect=SystemExit(42)) | ||||||||||||||||||||||||
| statement = "raise SystemExit(99)" | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| with self.assertRaises(SystemExit) as caught: | ||||||||||||||||||||||||
| self._run_interactive([statement, "x = 1"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertEqual(caught.exception.code, 42) | ||||||||||||||||||||||||
| hook.assert_called_once_with(statement) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_not_called_for_repl_commands(self): | ||||||||||||||||||||||||
| hook = MagicMock() | ||||||||||||||||||||||||
| self._run_interactive(["clear"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| hook.assert_not_called() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @force_not_colorized | ||||||||||||||||||||||||
| def test_hook_called_before_execution_exception(self): | ||||||||||||||||||||||||
| statement = "1 / 0" | ||||||||||||||||||||||||
| marker = "hook ran first" | ||||||||||||||||||||||||
| hook = MagicMock(side_effect=lambda statement: print(marker)) | ||||||||||||||||||||||||
| output, _ = self._run_interactive([statement], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| hook.assert_called_once_with(statement) | ||||||||||||||||||||||||
| marker_index = output.index(marker) | ||||||||||||||||||||||||
| traceback_index = output.index("ZeroDivisionError") | ||||||||||||||||||||||||
| self.assertLess(marker_index, traceback_index) | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Add an optional ``_pyrepl.simple_interact.statement_submitted_hook`` callback | ||
| that runs after input is submitted and before it is executed, | ||
| allowing external tools to implement features such as shell integration. | ||
| Hook failures are reported without replacing the last user exception. |
Uh oh!
There was an error while loading. Please reload this page.
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 appreciate the explicit description of the intent!
OTOH, this is a private API for (mainly) the benefit of VS Code. I'd make this shorter to reduce maintenance costs:
# not called for commands, return value is ignoredI wouldn't pay much attention to exception handling, the default convention is to report all errors somehow. What would be more concerning is if the errors weren't displayed.
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.
We can shorten this and remove the example, but keep the callback contract. External tools need to know when it runs, what it receives, and how errors behave.
“Submitted input” is more accurate than “complete statement”. Syntax errors and blank input reach the hook too. The exception description also needs to distinguish
ExceptionfromKeyboardInterruptandSystemExit.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.
Addressed in b7e9986fc46: Simplify comment to remove example.
Removed the example and shortened the comment while retaining the callback contract. The comment and NEWS entry now describe submitted input rather than a complete statement, and the comment distinguishes ordinary exceptions from KeyboardInterrupt and SystemExit.
Added coverage in 753ed835c8c: Add tests for KeyboardInterrupt and SystemExit.