Skip to content

Pythonrc: Don't register PyREPL hooks twice - #26145

Merged
Eduardo Villalpando Mello (edvilme) merged 2 commits into
mainfrom
pyrepl-pythonrc-hooks
Sep 10, 2026
Merged

Pythonrc: Don't register PyREPL hooks twice#26145
Eduardo Villalpando Mello (edvilme) merged 2 commits into
mainfrom
pyrepl-pythonrc-hooks

Conversation

@edvilme

@edvilme Eduardo Villalpando Mello (edvilme) commented Sep 10, 2026

Copy link
Copy Markdown

This pull request refactors the naming and usage of exception and display hooks in the pythonrc.py file to better reflect their association with VS Code integration. It also simplifies the REPL launch message and updates related tests to use the new hook names. Refactoring and renaming for clarity: * Renamed my_excepthook and my_displayhook methods in REPLHooks to vscode_excepthook and vscode_displayhook, and updated all references and assignments accordingly in pythonrc.py. * Updated test cases in test_shell_integration.py to use the new hook method names (vscode_displayhook and vscode_excepthook). [1] [2] Prompt message simplification: * Simplified the REPL launch message to dynamically use "Cmd" or "Ctrl" based on the platform, reducing duplicated code.

Fixes #26146

@edvilme Eduardo Villalpando Mello (edvilme) changed the title PyREPL: Don't register hooks twice Pythonrc: Don't register PyREPL hooks twice Sep 10, 2026
@bschnurr

Copy link
Copy Markdown
Member

🔒 Automated review in progress — Bill Schnurr (@bschnurr) is auto-reviewing this PR.

@bschnurr

Copy link
Copy Markdown
Member

Result: ⚠️ needs-more-tests

Verification details

Verification: Isolated verification observed failures that were not classified as caused by this PR: Dependency and test discovery preflight.

Summary: The targeted shell-integration suite passed all 4 collected tests. An ad-hoc probe confirmed each hook is registered exactly once and belongs to `PS1.hooks`. The initial discovery command failed because the container lacked Git metadata, not because of the PR. The core single-registration behavior has no committed regression test, so additional coverage is needed.

Test runs: 2 passed, 1 failed

  • Failed | unrelated to this PR | Dependency and test discovery preflight | set -eu
    printf '%s\n' '--- refs ---'
    git branch -a
    BASE_REF=''
    for ref in origin/main upstream/main main; do
    if git rev-parse --verify --quiet "$ref" >/dev/null; then BASE_REF="$ref"; break; fi
    done
    printf 'base_ref=%s\n' "$BASE_REF"
    if [ -n "$BASE_REF" ]; then
    BASE=$(git merge-base HEAD "$BASE_REF")
    printf 'merge_base=%s\n' "$BASE"
    git diff --name-status "$BASE" HEAD
    printf '%s\n' '--- focused diff ---'
    git diff --unified=3 "$BASE" HEAD -- python_files/pythonrc.py python_files/tests/test_shell_integration.py
    else
    git show --stat --oneline --decorate HEAD
    fi
    printf '%s\n' '--- runtime/dependencies ---'
    python --version
    python -c "import pytest; print('pytest', pytest.version)"
    python -m pytest --collect-only -q tests/test_shell_integration.py
  • Passed | Python shell integration tests | python -m pytest tests/test_shell_integration.py -q
  • Passed | Single hook registration probe | python - <<'PY'
    import enum
    import platform
    import sys
    import types
    from pathlib import Path

real_sys = sys

class TrackingSys(types.ModuleType):
def init(self, wrapped):
super().init('sys')
object.setattr(self, '_wrapped', wrapped)
object.setattr(self, 'assignments', {'excepthook': 0, 'displayhook': 0})

def __getattr__(self, name):
    return getattr(self._wrapped, name)

def __setattr__(self, name, value):
    if name in self.assignments:
        self.assignments[name] += 1
    object.__setattr__(self, name, value)

tracking_sys = TrackingSys(real_sys)
namespace = {'name': 'pythonrc_registration_probe'}
real_sys.modules['sys'] = tracking_sys
try:
exec(compile(Path('pythonrc.py').read_text(), 'pythonrc.py', 'exec'), namespace)
finally:
real_sys.modules['sys'] = real_sys

hooks = namespace['PS1'].hooks
assert tracking_sys.assignments == {'excepthook': 1, 'displayhook': 1}, tracking_sys.assignments
assert tracking_sys.excepthook.self is hooks
assert tracking_sys.displayhook.self is hooks
print('hook assignments:', tracking_sys.assignments)
print('both installed hooks belong to PS1.hooks')
PY

Dependency and test discovery preflight diagnostic output
--- refs ---
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

@bschnurr Bill Schnurr (bschnurr) left a comment

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.

Approved via Review Center.

@bschnurr Bill Schnurr (bschnurr) added the review-auto:approved Automated review: no blocking findings (approval posted). label Sep 10, 2026

@bschnurr Bill Schnurr (bschnurr) left a comment

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.

maybe add types in the future

@rchiodo Rich Chiodo (rchiodo) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved via Review Center.

@edvilme
Eduardo Villalpando Mello (edvilme) merged commit 303ba58 into main Sep 10, 2026
94 of 96 checks passed
@edvilme
Eduardo Villalpando Mello (edvilme) deleted the pyrepl-pythonrc-hooks branch September 10, 2026 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debt Code quality issues review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid duplicate PyREPL hook registration and clarify hook names

4 participants