gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process #102896

gaogaotiantian · 2023-03-22T02:00:07Z

Issue: code.interact() will exit the whole process when given exit() or quit() #102895

arhadthedev · 2023-04-07T07:34:46Z

@gvanrossum, @brettcannon (as the ones who committed to Lib/code.py the most), @birkenfeld (as the one who committed to Lib/pdb.py the most)

gvanrossum · 2023-04-07T15:23:25Z

Looks like a fine idea. I won’t be able to review. Make sure to add tests.

gaogaotiantian · 2023-04-07T17:14:17Z

Test is already added to make sure exit() has the same effect as EOFError()(Ctrl + D) when block_exit=True. I think in the original issue @ambv had some thoughts on the matter and @iritkatriel is normally the person reviewing code changes to pdb.

brettcannon · 2023-04-11T00:55:05Z

as the ones who committed to Lib/code.py the most

Me, really?!? I can't even remember when I last touched the code, so I don't feel up for doing a code review.

gaogaotiantian · 2023-04-16T05:13:16Z

Hi, I know this file has not been actively maintained for a while, but the code change is pretty straightforward and it's backward compatible. It will be at least beneficial for pdb's interact command. Maybe someone could take some time to review it? I can try to explain the logic if that helps. Thanks!

gaogaotiantian · 2023-04-25T00:23:56Z

@brandtbucher if you could take a look at this when you have some time :)

gvanrossum

This isn't a full review, sorry.

gvanrossum · 2023-04-25T06:15:49Z

Doc/library/code.rst

-   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local* is
-   provided, it is passed to the :class:`InteractiveConsole` constructor for
-   use as the default namespace for the interpreter loop.  The :meth:`interact`
-   method of the instance is then run with *banner* and *exitmsg* passed as the
-   banner and exit message to use, if provided.  The console object is discarded
-   after use.
+   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local*
+   or *block_exit* is provided, it is passed to the :class:`InteractiveConsole`
+   constructor for use as the default namespace for the interpreter loop.
+   The :meth:`interact` method of the instance is then run with *banner* and
+   *exitmsg* passed as the banner and exit message to use, if provided.
+   The console object is discarded after use.


We don't normally reflow the paragraph, it distracts the review. Just let the lines be uneven in length. :-)

Ah, I just thought the paragraph would be kind of ugly. Do you want me to convert this back to the uneven format? So only one line is modified?

Yes please! :)

This still needs to be done.

gvanrossum · 2023-04-25T06:16:51Z

Doc/library/code.rst

@@ -23,27 +23,33 @@ build applications which provide an interactive interpreter prompt.
   ``'__doc__'`` set to ``None``.


-.. class:: InteractiveConsole(locals=None, filename="<console>")
+.. class:: InteractiveConsole(locals=None, filename="<console>", block_exit=False)


Can you come up with a better name? To me, allow_exit=True or even just exit=True feels better.

So the difference between the option on and off is whether exit() would terminate the whole process, or just the interactive console. By block_exit I was trying to describe the option "blocks" the exit() function from exiting the whole process. exit() will always be allowed in the interactive console and it will always have effect. The only difference is to what level.

I would be happy to rename it to something better, but allow_exit probably does not describe the behavior well.

Yeah, this name is tricky. Maybe one of the other reviewers has an idea? If not, we can go with block_exit.

Yeah. Maybe ignore_exit=False?

When I see block here, it makes me think of a syntactic block (noun), or blocking (verb, as in "a blocking call"). It doesn't suggest "catch SystemExit" to me.

Well it's not "ignoring" exit(), like I said above, exit() has effect, just different when the option is toggled. How about something like local_exit? Meaning we will only "exit" the interpreter.

Yeah. Maybe ignore_exit=False?

When I see block here, it makes me think of a syntactic block (noun), or blocking (verb, as in "a blocking call"). It doesn't suggest "catch SystemExit" to me.

Circling back to this as I still think this PR has some value. Let's figure out a name for it. Do you like use_local_exit or local_exit? Or maybe a vague description like safe_exit? Or we can label its actual mechanism - exit_without_closing_stdin - that's the key thing we need - the existing behavior also raises SystemExit but stdin is closed so we are basically done.

brandtbucher · 2023-04-26T21:03:29Z

Lib/code.py

+        # process. exit and quit in builtins closes sys.stdin which makes
+        # it super difficult to restore


That's interesting! I didn't know about this additional wrinkle... thanks for the comment.

brandtbucher · 2023-04-26T21:09:45Z

Lib/code.py

+        self.eof = 'Ctrl-Z plus Return' if sys.platform == "win32" else \
+                   'Ctrl-D (i.e. EOF)'


Nit: I personally prefer an if-else to a ternary with a backslash. Also, site.py uses os.pathsep for this purpose... so let's maybe just be consistent with that?

Suggested change

self.eof = 'Ctrl-Z plus Return' if sys.platform == "win32" else \

'Ctrl-D (i.e. EOF)'

if os.sep == '\\':

self.eof = "Ctrl-Z plus Return"

else:

self.eof = "Ctrl-D (i.e. EOF)"

I'm okay with the if/else block instead of an inline statement, but I kind of changed the OS check on purpose.

The fundamental thing we want to know here is what's the EOF on a specific system, and the best effort we can make is to get the OS type - if it's Windows then it's Ctrl+Z + Return. os.sep is a design decision(finger print?) for Windows, the logic would be: os.sep == '\\' -> "this is Windows!". The seperator itself has nothing to do with what the eof directly, it just happens to be linked together because of Windows.

For similar checks, there are a lot of usage of sys.platform == "win32" in the standard library, and almost only site.py uses os.sep to figure out the OS platform. So I think sys.platform is actually preferred in this case.

Could the site.py code be a holdover from DOS, which isn’t Windows but still uses \ ? Presumably other antique OS’es using \ also used ^Z ? Not that it matters today. :-)

Too bad I'm not young enough to ask the question "what is DOS". That's a possibility, but the connection between \\ and Ctrl+Z is still indirect. I don't know if we can build modern Python on ancient OS anymore, always assume that Windows is the only outlier.

brandtbucher · 2023-04-26T21:13:32Z

Lib/code.py

@@ -276,8 +315,20 @@ def raw_input(self, prompt=""):
        return input(prompt)


+class Quitter:


Maybe just add a short docstring explaining that this is based on _sitebuiltins.Quitter, and how it is different.

Perhaps it's even worth just subclassing Quitter and overriding __call__? Not sure. These are both pretty simple, but also pretty tightly coupled. Your call.

Subclassing Quitter won't give a lot of benefit, because the OS checking is outside the original Quitter class. We could calculate eof outside of the __init__ function, it just seems like unnecessary. The class itself is already very compact and easy to understand.

brandtbucher · 2023-04-26T21:15:06Z

Doc/library/code.rst

@@ -23,27 +23,33 @@ build applications which provide an interactive interpreter prompt.
   ``'__doc__'`` set to ``None``.


-.. class:: InteractiveConsole(locals=None, filename="<console>")
+.. class:: InteractiveConsole(locals=None, filename="<console>", block_exit=False)


Yeah. Maybe ignore_exit=False?

When I see block here, it makes me think of a syntactic block (noun), or blocking (verb, as in "a blocking call"). It doesn't suggest "catch SystemExit" to me.

brandtbucher · 2023-04-26T21:15:10Z

Doc/library/code.rst

-   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local* is
-   provided, it is passed to the :class:`InteractiveConsole` constructor for
-   use as the default namespace for the interpreter loop.  The :meth:`interact`
-   method of the instance is then run with *banner* and *exitmsg* passed as the
-   banner and exit message to use, if provided.  The console object is discarded
-   after use.
+   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local*
+   or *block_exit* is provided, it is passed to the :class:`InteractiveConsole`
+   constructor for use as the default namespace for the interpreter loop.
+   The :meth:`interact` method of the instance is then run with *banner* and
+   *exitmsg* passed as the banner and exit message to use, if provided.
+   The console object is discarded after use.


Yes please! :)

gvanrossum

Looks like several review comments didn't see action yet?

Regarding the name for the flag, was it already decided that we can't just always overwrite those two builtins? (Why are they closing stdin anyways? I don't recall; maybe you can find a clue in the code? It may be too long ago for the commit history to be useful.)

If we do need this feature, since the name has proved tricky, let's go with local_exit. The name probably doesn't matter too much since the feature feels pretty obscure.

gvanrossum · 2023-09-12T00:36:53Z

Doc/library/code.rst

-   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local* is
-   provided, it is passed to the :class:`InteractiveConsole` constructor for
-   use as the default namespace for the interpreter loop.  The :meth:`interact`
-   method of the instance is then run with *banner* and *exitmsg* passed as the
-   banner and exit message to use, if provided.  The console object is discarded
-   after use.
+   the :meth:`InteractiveConsole.raw_input` method, if provided.  If *local*
+   or *block_exit* is provided, it is passed to the :class:`InteractiveConsole`
+   constructor for use as the default namespace for the interpreter loop.
+   The :meth:`interact` method of the instance is then run with *banner* and
+   *exitmsg* passed as the banner and exit message to use, if provided.
+   The console object is discarded after use.


This still needs to be done.

Doc/library/code.rst

gaogaotiantian · 2023-09-12T05:13:03Z

Looks like several review comments didn't see action yet?

Regarding the name for the flag, was it already decided that we can't just always overwrite those two builtins? (Why are they closing stdin anyways? I don't recall; maybe you can find a clue in the code? It may be too long ago for the commit history to be useful.)

If we do need this feature, since the name has proved tricky, let's go with local_exit. The name probably doesn't matter too much since the feature feels pretty obscure.

Yes, I waited for the name confirmation as almost all files require that name. I updated all the files to use the name local_exit.

By overwriting those two builtins, what do you mean? Change the builtin classes? They close stdin because

Shells like IDLE catch the SystemExit, but listen when their stdin wrapper is closed.

gvanrossum · 2023-09-12T17:56:01Z

@brandtbucher: Could you take over review here? I don't want this to languish for another 5 months (but neither is it urgent).

brandtbucher · 2023-09-12T18:14:06Z

Yep, I can.

MaxwellDupre

The changes LGTM but I see Merging is blocked, hence suggest this is moved up to near the top.

gaogaotiantian · 2023-10-07T05:57:54Z

Taking a note here that this would fix #85268

brandtbucher · 2023-10-18T18:36:11Z

Alright, I think this has marinated for long enough. :)

…it() from terminating the whole process (pythonGH-102896)

Overwrite exit and quit in code shell

d992ed1

bedevere-bot mentioned this pull request Mar 22, 2023

code.interact() will exit the whole process when given exit() or quit() #102895

Closed

bedevere-bot added the awaiting review label Mar 22, 2023

📜🤖 Added by blurb_it.

67038e3

arhadthedev added the stdlib Python modules in the Lib dir label Mar 22, 2023

gaogaotiantian added 2 commits March 28, 2023 20:26

Add block_exit for opt-in changing exit behavior

68dbfc1

Add test and docs

bc5c110

gaogaotiantian changed the title ~~gh-102895 Replace exit and quit in code.interact for a reasonable and unified behavior~~ gh-102895 Add an option block_exit in code.interact to block exit() from terminating the whole process Apr 7, 2023

gaogaotiantian added 2 commits April 6, 2023 23:51

Fix trailing space

9fa32a1

Update 2023-03-22-02-01-30.gh-issue-102895.HiEqaZ.rst

eedb3e2

gvanrossum reviewed Apr 25, 2023

View reviewed changes

brandtbucher self-requested a review April 25, 2023 17:51

brandtbucher reviewed Apr 26, 2023

View reviewed changes

gvanrossum reviewed Sep 12, 2023

View reviewed changes

gaogaotiantian added 5 commits September 11, 2023 21:53

Rename block_exit to local_exit and polish docs

3c58bf8

Update versionchanged

1916273

Modified Quitter according to the review

56d7237

Update names

2dc19a4

Update name

91befb5

gaogaotiantian changed the title ~~gh-102895 Add an option block_exit in code.interact to block exit() from terminating the whole process~~ gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process Sep 12, 2023

gaogaotiantian added 2 commits September 11, 2023 22:07

Make documentation more accurate

b69feca

Update news

818fbac

Merge branch 'main' into code-interact-exit

ada2ce1

brandtbucher self-requested a review September 12, 2023 18:14

brandtbucher self-assigned this Sep 12, 2023

MaxwellDupre suggested changes Sep 15, 2023

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Sep 15, 2023

brandtbucher merged commit e6eb8ca into python:main Oct 18, 2023

bedevere-app bot removed the awaiting core review label Oct 18, 2023

gaogaotiantian deleted the code-interact-exit branch October 18, 2023 18:52

This was referenced Oct 18, 2023

Need command to exit PDB interactive shell #85268

Closed

gh-102980: Redirect pdb interact command, add tests and improve docs #111194

Merged

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythonGH-102895 Add an option local_exit in code.interact to block ex…

b929f8a

…it() from terminating the whole process (pythonGH-102896)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythonGH-102895 Add an option local_exit in code.interact to block ex…

49b6660

…it() from terminating the whole process (pythonGH-102896)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process #102896

gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process #102896

gaogaotiantian commented Mar 22, 2023 •

edited by bedevere-bot

Loading

arhadthedev commented Apr 7, 2023

gvanrossum commented Apr 7, 2023

gaogaotiantian commented Apr 7, 2023

brettcannon commented Apr 11, 2023

gaogaotiantian commented Apr 16, 2023 •

edited

Loading

gaogaotiantian commented Apr 25, 2023

gvanrossum left a comment

gvanrossum Apr 25, 2023

gaogaotiantian Apr 25, 2023

brandtbucher Apr 26, 2023

gvanrossum Sep 12, 2023

gvanrossum Apr 25, 2023

gaogaotiantian Apr 25, 2023

gvanrossum Apr 25, 2023

brandtbucher Apr 26, 2023

gaogaotiantian Apr 27, 2023

gaogaotiantian Sep 11, 2023

brandtbucher Apr 26, 2023

brandtbucher Apr 26, 2023

gaogaotiantian Apr 27, 2023

gvanrossum Apr 29, 2023

gaogaotiantian Apr 29, 2023

brandtbucher Apr 26, 2023

gaogaotiantian Apr 27, 2023

brandtbucher Apr 26, 2023

brandtbucher Apr 26, 2023

gvanrossum left a comment

gvanrossum Sep 12, 2023

gaogaotiantian commented Sep 12, 2023

gvanrossum commented Sep 12, 2023

brandtbucher commented Sep 12, 2023

MaxwellDupre left a comment

gaogaotiantian commented Oct 7, 2023

brandtbucher commented Oct 18, 2023

		# process. exit and quit in builtins closes sys.stdin which makes
		# it super difficult to restore

		self.eof = 'Ctrl-Z plus Return' if sys.platform == "win32" else \
		'Ctrl-D (i.e. EOF)'

-        self.eof = 'Ctrl-Z plus Return' if sys.platform == "win32" else \
-                   'Ctrl-D (i.e. EOF)'
+        if os.sep == '\\':
+            self.eof = "Ctrl-Z plus Return"
+        else:
+            self.eof = "Ctrl-D (i.e. EOF)"

		@@ -276,8 +315,20 @@ def raw_input(self, prompt=""):
		return input(prompt)


		class Quitter:

gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process #102896

gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process #102896

Conversation

gaogaotiantian commented Mar 22, 2023 • edited by bedevere-bot Loading

arhadthedev commented Apr 7, 2023

gvanrossum commented Apr 7, 2023

gaogaotiantian commented Apr 7, 2023

brettcannon commented Apr 11, 2023

gaogaotiantian commented Apr 16, 2023 • edited Loading

gaogaotiantian commented Apr 25, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaogaotiantian commented Sep 12, 2023

gvanrossum commented Sep 12, 2023

brandtbucher commented Sep 12, 2023

MaxwellDupre left a comment

Choose a reason for hiding this comment

gaogaotiantian commented Oct 7, 2023

brandtbucher commented Oct 18, 2023

gaogaotiantian commented Mar 22, 2023 •

edited by bedevere-bot

Loading

gaogaotiantian commented Apr 16, 2023 •

edited

Loading