Skip to content
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.

Sign up for GitHub

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

GH-121584: Toggle helper instead of nesting instances when using key bindings #121668

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+27 −4
Open

GH-121584: Toggle helper instead of nesting instances when using key bindings #121668

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e2fa6d
Toggle helper instead of nesting instances when using key bindings
saucoide Jul 13, 2024
d957489
Merge branch 'main' into repl-toggle-help
saucoide Jul 13, 2024
b9e538e
Add NEWS entry
pablogsal Jan 21, 2025
a04c214
Merge branch 'main' into repl-toggle-help
pablogsal Jan 21, 2025
1901ba8
change sigint by raising a keyboard interrupt
saucoide Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Lib/_pyrepl/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,16 @@ def do(self) -> None:
class help(Command):
def do(self) -> None:
import _sitebuiltins
import signal

with self.reader.suspend():
self.reader.msg = _sitebuiltins._Helper()() # type: ignore[assignment, call-arg]
if self.reader.help_mode:
self.reader.help_mode = False
signal.raise_signal(signal.SIGINT)
Copy link
Member

@pablogsal pablogsal Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hummm...I am worried by this. Why are we raising SIGINT here? This looks very error prone. We should not be handling state resets via signals

Copy link
Contributor Author

@saucoide saucoide Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed a way to get out of the help's input prompt here

    def interact(self):
        self.output.write('\n')
        while True:
            try:
                request = self.getline('help> ')
                if not request: break
            except (KeyboardInterrupt, EOFError):
                break
            request = request.strip()

            # Make sure significant trailing quoting marks of literals don't
            # get deleted while cleaning input
            if (len(request) > 2 and request[0] == request[-1] in ("'", '"')
                    and request[0] not in request[1:-1]):
                request = request[1:-1]
            if request.lower() in ('q', 'quit', 'exit'): break
            if request == 'help':
                self.intro()
            else:
                self.help(request)

    def getline(self, prompt):
        """Read one line, using input() when appropriate."""
        if self.input is sys.stdin:
            return input(prompt)
        else:
            self.output.write(prompt)
            self.output.flush()
            return self.input.readline()

I've swapped it now for a KeyboardInterrupt, would that be ok?

"I have made the requested changes; please review again"

Copy link
Member

@pablogsal pablogsal Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct approach. Relying on signals for this is super error prone in my opinion. Not only that but testing this that way is going to be a nightmare taking into account how signals behave in different machines. Also if the user has handles already installed you will be executing that code which may have side effects that the user didn't trigger.

This probably requires some heavier changes but I don't think we want to deal with the chaos of control flow by signals.

WDYT @ambv

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newest version of the code doesn't raise_signal anymore but raises a regular exception with KeyboardInterrupt. This is a bit weird, but we already do this ourselves in commands.ctrl_c. That command is mapped to \x03 in reader (used by the asyncio REPL) and in the ExceptHookHandler (used for threading.excepthook so that exceptions from threads created in the REPL format okay).

While in both cases the command works around the fact that there's no good way to interrupt input from another thread, the way we actually interrupt the input is by raising KeyboardInterrupt by hand in the command.

All I'm saying is that there's precedent. I would want to test this with the asyncio REPL.

else:
self.reader.help_mode = True
with self.reader.suspend():
self.reader.msg = _sitebuiltins._Helper()() # type: ignore[assignment, call-arg]
self.reader.help_mode = False


class invalid_key(Command):
Expand Down
3 changes: 2 additions & 1 deletion Lib/_pyrepl/reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ class Reader:
dirty: bool = False
finished: bool = False
paste_mode: bool = False
help_mode: bool = False
in_bracketed_paste: bool = False
commands: dict[str, type[Command]] = field(default_factory=make_default_commands)
last_command: type[Command] | None = None
Expand Down Expand Up @@ -665,7 +666,7 @@ def suspend(self) -> SimpleContextManager:
self.restore()
yield
finally:
for arg in ("msg", "ps1", "ps2", "ps3", "ps4", "paste_mode"):
for arg in ("msg", "ps1", "ps2", "ps3", "ps4", "paste_mode", "help_mode"):
setattr(self, arg, prev_state[arg])
self.prepare()

Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_pyrepl/support.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def prepare_reader(console: Console, **kwargs):
reader = ReadlineAlikeReader(console=console, config=config)
reader.more_lines = partial(more_lines, namespace=None)
reader.paste_mode = True # Avoid extra indents
reader.help_mode = False

def get_prompt(lineno, cursor_on_line) -> str:
return ""
Expand Down
14 changes: 14 additions & 0 deletions Lib/test/test_pyrepl/test_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,20 @@ def test_up_arrow_after_ctrl_r(self):
reader, _ = handle_all_events(events)
self.assert_screen_equals(reader, "")

def test_help_toggles_instead_of_nesting(self):

events = [
Event(evt="key", data="f1", raw=bytearray(b"")),
]

no_paste_reader = functools.partial(
prepare_reader,
paste_mode=False,
help_mode=True,
)
reader, _ = handle_all_events(events, prepare_reader=no_paste_reader)
self.assertFalse(reader.help_mode)

def test_newline_within_block_trailing_whitespace(self):
# fmt: off
code = (
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2025-01-21-19-46-20.gh-issue-121584.O3lEdm.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Toggle helper instead of nesting instances when using key bindings in the
new REPL.
Loading