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.

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

bpo-44603: Exit the interpreter if the user types "exit" #27096

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jul 12, 2021

Python/pythonrun.c Outdated Show resolved Hide resolved
Python/pythonrun.c Outdated Show resolved Hide resolved
@pablogsal pablogsal marked this pull request as ready for review July 12, 2021 16:29
@gpshead
Copy link
Member

gpshead commented Jul 14, 2021

LGTM, I suggest reopening this (button clicked).

From a testing perspective, we just want to make sure it doesn't interfere with with what uber interactive console add-on's like IPython already implement that do this.

@gpshead gpshead reopened this Jul 14, 2021
@gpshead
Copy link
Member

gpshead commented Jul 14, 2021

From a PR standpoint I think the thing we need was covered in your bug comment:

"if there's a single AST node that is a name and the name is "exit" or "quit", the REPL inspects locals and globals to see if the object referred to is a Quitter, and if so it exits"

We should be able to add unittest coverage for this in test_repl.py including equivalent test coverage that such exit/quit treatment is not present outside the repl.

@gpshead gpshead self-requested a review July 14, 2021 03:13
@gpshead gpshead added the type-feature A feature request or enhancement label Jul 14, 2021
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

For the purpose of the review, I will be neutral on the proposal. But I do notice that the special behavior is limited to entering exactly 'quit\n' or 'exit\n'. This is stricter than current ^Z behavior.

>>> a^Z
  File "<stdin>", line 1
    a→
     ^
SyntaxError: invalid syntax
>>> ^Zb

f:\dev\3x>

Exiting instead of also raising SyntaxError may be a bug.

Changes:

  1. The reprs of both functions should be changed to eliminate the ()s.
  2. The code class imitating the REPL should imitate this also. If no syntax error, run same tests on result as ast.parse. But do in a separate function, as here, so subclass can easily disable with def is_exit(code): return False.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

}
return PyUnicode_CompareWithASCIIString(expr->v.Name.id, "exit") == 0 ||
PyUnicode_CompareWithASCIIString(expr->v.Name.id, "quit") == 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Tests of the object associated with the names is needed to not turn them into semi-keywords, as in the the following.

>>> exit = 3
>>> exit

f:\dev\3x>

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 17, 2021
@Privat33r-dev
Copy link
Contributor

@pablogsal this PR looks great, any chance you will continue working on it? :)

@pablogsal
Copy link
Member Author

@pablogsal this PR looks great, any chance you will continue working on it? :)

There is still no agreement on the solution. Furthermore we are working on a new REPL for CPython that won't need this hack so probably that's the better rule if we decide to go that way

@gpshead gpshead marked this pull request as draft March 6, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PR or inactive for long period of time. type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants