gh-121804: always show error location for SyntaxError's in basic repl - #123202
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
* 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 |
@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 |
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 |
picnixz
left a comment
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.
|
Thanks @skirpichev for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…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. |
|
Thanks for the pr @skirpichev and thanks @picnixz for your comments 🚀 |
|
@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! |
|
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?
|

SyntaxErroris raised #121804