GH-121584: Toggle helper instead of nesting instances when using key bindings #121668
Conversation
Adds a flag to check if the helper is active, interrupting it instead of opening a new instance if so
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
@ambv i just saw that the toggle behavior for help is mentioned explicitly in the pep, any chance this will get merged or is that part going to be reworked anyway and better not to introduce more stuff? |
Lib/_pyrepl/commands.py
Outdated
| 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Adds a flag to check if the helper is active, interrupting it instead of opening a new instance if so
This is to stop F1 from opening multiple instances of help you need to quit individually before returning to the repl, maybe the toggling part with the sigint is not needed, but I figured
paste_modebehaves that way so maybe this can too?