| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Sorry @skirpichev but I think this is probably too much complexity (even if the code it's not too difficult to follow) for the fallback that it's not going to be triggered most of the time. Let me check with more core devs to get their input first @ambv @lysnikolaou what do toy think? |
Sorry, something went wrong.
* use _PyErr_GetRaisedException * and PyType_IsSubtype
|
If nobody answers by Wednesday next week, ping me and we can merge it. After looking at it I think it may be fine 👍 Ensure to run a bunch of ref leaks manual checks by hand, please. Unfortunately running them in the REPL it's not trivial, but yo can manually compare the output of sys.gettotalrefcount before and after raising. |
Sorry, something went wrong.
@pablogsal, I think that there is no reference leaks, but I can't justify this by sys.gettotalrefcount() output: $ PYTHON_BASIC_REPL=1 ./python -q
>>> import gc, sys
>>> sys.gettotalrefcount()
61809
>>> gc.collect()
106
>>> sys.gettotalrefcount()
60837
>>> def f(x, x): ...
...
File "<stdin>-4", line 1
def f(x, x): ...
^
SyntaxError: duplicate argument 'x' in function definition
>>> gc.collect()
0
>>> sys.gettotalrefcount()
60871
>>> gc.collect()
0
>>> sys.gettotalrefcount()
60883 |
Sorry, something went wrong.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…pichev/cpython into syntaxerr-location-basicrepl-121804
…e-121804.r5K3PS.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sorry for not replying earlier. I was out on vacation. I don't have a strong opinion either way, but I agree with the sentiment that might be a bit too complex. It improves a subset of SyntaxErrors for a subset of users that will continue to use the old REPL after they've switched to 3.13 (I don't think this could be backported to earlier versions) with a fairly complicated approach. |
Sorry, something went wrong.
There was a problem hiding this comment.
Personally, I think it's better for the user's experience to show syntax errors correctly, but I don't know whether the basic REPL will be used a lot or not and whether those kind of errors are shown a lot or not. So I'm +0.5.
Sorry, something went wrong.
|
Thanks @skirpichev for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
…c repl (pythonGH-123202) (cherry picked from commit 6822cb2) Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
|
GH-123631 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
|
Thanks for the pr @skirpichev and thanks @picnixz for your comments 🚀 |
Sorry, something went wrong.
|
@skirpichev For next time, please could you upgrade to blurb 1.2+ so we don't get spaces in the news file path? https://discuss.python.org/t/new-blurb-1-2-please-upgrade/59159 No need to change this news file. Thanks! |
Sorry, something went wrong.
|
On Wed, Sep 04, 2024 at 04:15:07AM -0700, Hugo van Kemenade wrote:
***@***.*** For next time, please could you upgrade to blurb 1.2+ so we
don't get spaces in the news file path?
Thanks for remind this. I did upgrade of my toolchain.
But, perhaps, we should update devguide to specify minimal version of
blurb or enforce upgrade to latest stable version?
|
Sorry, something went wrong.
|
I plan on enforcing it via CI at some point, maybe after 3.13.0 or 3.14.0a1? Something like hugovk@e4cc2f7 -> https://github.com/hugovk/cpython/actions/runs/10576194271/job/29301498185 We can't enforce a minimal blurb version as such, people can also use Blurb It (already updated to use underscores) or write them by hand. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.