-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-111201: A new Python REPL #111567
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
gh-111201: A new Python REPL #111567
Conversation
pablogsal
commented
Oct 31, 2023
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: A new Python REPL #111201
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You add many env vars. Can you document them in Doc/using/cmdline.rst?
Usually, env var names start with PYTHON. Should the variables be renamed?
Also, Python ignores env var starting with PYTHON if sys.flags.ignore_environment.
manually. | ||
|
||
Note that there is also a built-in module _minimal_curses which will | ||
hide this one if compiled in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it part of this PR? If not, do you consider adding it or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds like this comment was carried over from pypy but doesn't apply here.
@vstinner the PR is still draft, please wait until we mark it as finished before reviewing as most of this code may change |
…importing too much
c34adb8
to
702b59e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a What's New entry.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
There is already one (Misc/NEWS.d/next/Core and Builtins/2024-04-28-00-41-17.gh-issue-111201.cQsh5U.rst) |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@hugovk @JelleZijlstra I have implemented your suggestions. If you have some time, do you mind reviewing again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs changes look good.
One suggestion: this PR adds the main docs to the tutorial, we should probably add something to the reference, but this could be a followup.
Please also add a What's New entry.
There is already one (Misc/NEWS.d/next/Core and Builtins/2024-04-28-00-41-17.gh-issue-111201.cQsh5U.rst)
That's the changelog, this change definitely needs highlighting in https://docs.python.org/3.13/whatsnew/3.13.html but it can also be in a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool to see this!
Just to check, was it intentional that you didn't port PyPy's _pyrepl
tests? There's not a huge amount of tests there, admittedly.
I am philosophically a little bit worried that people will start using and relying on _pyrepl
internal details, which are now slightly different between PyPy and CPython, right? This could lead to PyPy's life getting somewhat harder in this area. As usual, it's hard to stop people from doing that, of course.
self.restore() | ||
yield | ||
finally: | ||
for arg in ("msg", "ps1", "ps2", "ps3", "ps4", "paste_mode"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a discrepancy between how prev_state
is constructed (which uses fields(self)
and how it is used in the finally block (listing the attributes explicitly)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It evolved into this, which you're right would now be cleaner to just reuse the list of fields.
I originally started with just dict(__dict__)
but when I added typing with slots=True
the __dict__
was gone so I modified it to use fields()
instead.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
|
|
Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Marta Gómez Macías <mgmacias@google.com> Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>