◐ Shell
reader mode source ↗
Skip to content

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

Merged
brandtbucher merged 14 commits into
python:mainfrom
gaogaotiantian:code-interact-exit
Oct 18, 2023
Merged

gh-102895 Add an option local_exit in code.interact to block exit() from terminating the whole process#102896
brandtbucher merged 14 commits into
python:mainfrom
gaogaotiantian:code-interact-exit

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Mar 22, 2023

Copy link
Copy Markdown
Member

@arhadthedev arhadthedev added the stdlib Standard Library Python modules in the Lib/ directory label Mar 22, 2023
@gaogaotiantian gaogaotiantian changed the title gh-102895 Replace exit and quit in code.interact for a reasonable and unified behavior Apr 7, 2023
@arhadthedev

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

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

@gaogaotiantian

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

gaogaotiantian commented Apr 16, 2023

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

@gvanrossum gvanrossum 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.

Hide comment

This isn't a full review, sorry.

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

@gvanrossum gvanrossum 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.

Hide comment

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.

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

@brandtbucher

Copy link
Copy Markdown
Member

Yep, I can.

@brandtbucher brandtbucher self-requested a review September 12, 2023 18:14
@brandtbucher brandtbucher self-assigned this Sep 12, 2023

@MaxwellDupre MaxwellDupre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

@gaogaotiantian

Copy link
Copy Markdown
Member Author

Taking a note here that this would fix #85268

@brandtbucher

Copy link
Copy Markdown
Member

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

@brandtbucher brandtbucher merged commit e6eb8ca into python:main Oct 18, 2023
@gaogaotiantian gaogaotiantian deleted the code-interact-exit branch October 18, 2023 18:52
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants