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
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 |
|---|---|---|
|
|
@@ -33,6 +33,25 @@ | |
| from .readline import _get_reader, multiline_input, append_history_file | ||
|
|
||
|
|
||
| # Called with the complete statement, which may span multiple lines, after the | ||
| # user submits it but before it is executed. The hook is not called for PyREPL | ||
| # commands such as ``clear``. Exceptions raised by the hook are displayed but | ||
| # do not prevent the statement from running. Its return value is ignored. | ||
| # External tools such as IDEs can install a hook to augment the behavior of the | ||
| # REPL. | ||
| # | ||
| # For example, an IDE can mark the start of command execution: | ||
| # | ||
| # from _pyrepl import simple_interact | ||
| # | ||
| # simple_interact.statement_submitted_hook = lambda x: print("\x1b]633;C\x07", end="") | ||
| # | ||
| # In the REPL: | ||
| # >>> print("Hello, world!") | ||
| # \x1b]633;C\x07Hello, world! | ||
| statement_submitted_hook = None | ||
|
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 appreciate the explicit description of the intent!
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. 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 |
||
|
|
||
|
|
||
| _error: tuple[type[Exception], ...] | type[Exception] | ||
| try: | ||
| from .unix_console import _error | ||
|
|
@@ -150,6 +169,12 @@ def maybe_run_command(statement: str) -> bool: | |
| if maybe_run_command(statement): | ||
| continue | ||
|
|
||
| if statement_submitted_hook is not None: | ||
| try: | ||
| statement_submitted_hook(statement) | ||
|
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. Nit: I think it would make sense to heat-treat What would you rather receive on the hook in the case below, >>> def f():
... 'there will be some final indent'
...
-hook called with "def f():\n 'there will be some final indent'\n "
+hook called with "def f():\n 'there will be some final indent'\n" |
||
| except Exception: | ||
| console.showtraceback() | ||
|
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. It's now easy to misread a traceback from the hook's exception as an exception that comes from running the user code: >>> from functools import singledispatch
Traceback (most recent call last):
File "<python-input-7>", line 2, in hook
1/0
~^~
ZeroDivisionError: division by zero
>>>(I've set the hook in the session, but even if there's internal VSCode paths included in the traceback that are telling this is some hook hackery, I still think this could be improved.) Additionally, that traceback is displayed before the user code exception traceback. I'd prefer:
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 agree the traceback needs a clear hook label, but I prefer to keep it visible.
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. There is a behavior issue with |
||
|
|
||
| input_name = f"<python-input-{input_n}>" | ||
| more = console.push(_strip_final_indent(statement), filename=input_name, _symbol="single") # type: ignore[call-arg] | ||
| assert not more | ||
|
|
||
| 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 _pyrepl import simple_interact | ||||||||||||||||||||||||
| from _pyrepl.console import InteractiveColoredConsole | ||||||||||||||||||||||||
| from _pyrepl.simple_interact import ( | ||||||||||||||||||||||||
| _more_lines, | ||||||||||||||||||||||||
|
|
@@ -358,3 +359,78 @@ 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 |
||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||
| 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") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @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("RuntimeError: hook error", output) | ||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def test_hook_return_value_is_ignored(self): | ||||||||||||||||||||||||
| hook = MagicMock(return_value=True) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| _, namespace = self._run_interactive(["x = 1"], hook) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| self.assertEqual(namespace["x"], 1) | ||||||||||||||||||||||||
|
Comment on lines
+413
to
+418
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'm not sure there's a reliable way to test that the return value is ignored. I'd remove this case from being tested. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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,3 @@ | ||
| Add an optional ``_pyrepl.simple_interact.statement_submitted_hook`` callback | ||
| that runs after a complete statement is submitted and before it is executed, | ||
| allowing external tools to implement features such as shell integration. |
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.
Oh, one more thing.
This isn't exactly correct. I think
a b cis not a "complete statement", no?Perhaps we could think if there's a better name?