Skip to content

gh-122273: Support PyREPL history in Windows - #122274

Closed
devdanzin wants to merge 13 commits into
python:mainfrom
devdanzin:pyrepl_history_windows
Closed

gh-122273: Support PyREPL history in Windows#122274
devdanzin wants to merge 13 commits into
python:mainfrom
devdanzin:pyrepl_history_windows

Conversation

@devdanzin

@devdanzindevdanzin commented Jul 25, 2024

Copy link
Copy Markdown
Member

This PR aims to add support for recording and loading command history in Windows, using _pyrepl.readline to support handling the history file.

@devdanzin
devdanzin marked this pull request as ready for review July 25, 2024 19:39
@JeffersGlass

Copy link
Copy Markdown
Contributor

Gave this a try, and it does appear to work as expected, at least on a stock Windows 10 build:

image

@vstinner

Copy link
Copy Markdown
Member

@devdanzin: There is now a merge conflict after my latest site change. Would you mind to update your PR?

Comment threadLib/site.py Outdated
import readline
real_readline = True
except ImportError:
import _pyrepl.readline as readline

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should not import if PYTHON_BASIC_REPL is set.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This part replaces an unconditional import of readline:

 import atexit
try:
- import readline+ try:+ import readline+ real_readline = True+ except ImportError:+ import _pyrepl.readline as readline+ real_readline = False
import rlcompleter # noqa: F401

Then readline is used in a couple places without checking it's a valid module. Should I set readline = None for the PYTHON_BASIC_REPL case and check against it being falsy where needed?

Should look something like:

diff --git a/Lib/site.py b/Lib/site.py
index 9d3352c70e3..a27373000fa 100644
--- a/Lib/site.py+++ b/Lib/site.py@@ -502,12 +502,13 @@ def register_readline():
import readline
real_readline = True
except ImportError:
- import _pyrepl.readline as readline+ readline = None
real_readline = False
import rlcompleter # noqa: F401
if PYTHON_BASIC_REPL:
CAN_USE_PYREPL = False
else:
+ import _pyrepl.readline as readline
from _pyrepl.main import CAN_USE_PYREPL
if real_readline:
import _pyrepl.unix_console
@@ -521,9 +522,10 @@ def register_readline():
# Reading the initialization (config) file may not be enough to set a
# completion key, so we set one first and then read the file.
- if getattr(readline, "backend", None) == 'editline':+ backend = getattr(readline, "backend", None)+ if backend == 'editline':
readline.parse_and_bind('bind ^I rl_complete')
- else:+ elif backend:
readline.parse_and_bind('tab: complete')
try:
@@ -536,7 +538,7 @@ def register_readline():
# want to ignore the exception.
pass
- if readline.get_current_history_length() == 0:+ if readline and readline.get_current_history_length() == 0:
# If no history was loaded, default to .python_history,
# or PYTHON_HISTORY.
# The guard is necessary to avoid doubling history size at
@@ -553,13 +555,15 @@ def register_readline():
exceptions = OSError
try:
- readline_module.read_history_file(history)+ if readline:+ readline_module.read_history_file(history)
except exceptions:
pass
def write_history():
try:
- readline_module.write_history_file(history)+ if readline:+ readline_module.write_history_file(history)
except (FileNotFoundError, PermissionError):
# home directory does not exist or is not writable
# https://bugs.python.org/issue19891

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I've committed a version of the approach above.

Comment threadLib/site.py Outdated
@vstinner

Copy link
Copy Markdown
Member

I'm not fully satisfied by proposed change, so I proposed a PR based somehow on this one: PR gh-127141.

@devdanzin

Copy link
Copy Markdown
MemberAuthor

Closing in favor of PR #127141, which does it better.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@devdanzin@JeffersGlass@vstinner