◐ Shell
reader mode source ↗
Skip to content

gh-106922: Support multi-line error locations in traceback (attempt 2)#112097

Merged
pablogsal merged 6 commits into
python:mainfrom
williamwen42:multiline-traceback-attempt-2
Dec 1, 2023
Merged

gh-106922: Support multi-line error locations in traceback (attempt 2)#112097
pablogsal merged 6 commits into
python:mainfrom
williamwen42:multiline-traceback-attempt-2

Conversation

@williamwen42

@williamwen42 williamwen42 commented Nov 15, 2023 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Attempt 2 at #109589 - the C changes are no longer required, so only python files and tests are touched. Otherwise, the method to output multi-line errors remains unchanged compared to the original PR.

@bedevere-app bedevere-app Bot mentioned this pull request Nov 15, 2023
Closed
@bedevere-app

bedevere-app Bot commented Nov 15, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@williamwen42

williamwen42 commented Nov 15, 2023

Copy link
Copy Markdown
Contributor Author

cc @pablogsal @1st1 @lysnikolaou

@pablogsal pablogsal self-assigned this Nov 15, 2023
@pablogsal

pablogsal commented Nov 17, 2023

Copy link
Copy Markdown
Member

@williamwen42 Seems that there are a bunch of doctests that still fail, can you take a look?

@williamwen42 williamwen42 force-pushed the multiline-traceback-attempt-2 branch from 9d9a510 to 4094c93 Compare November 17, 2023 19:32
pablogsal
pablogsal reviewed Nov 18, 2023
View reviewed changes

@pablogsal pablogsal 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 looks very good. Great work. I made a first pass over the code.

1 hidden conversation Load more…
@pablogsal

pablogsal commented Nov 18, 2023

Copy link
Copy Markdown
Member

@lysnikolaou Can you take a look when you have some time? This is a considerable ammount of code and I think it needs extra eyes

lysnikolaou
lysnikolaou reviewed Nov 20, 2023
View reviewed changes

@lysnikolaou lysnikolaou 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 look very good in general indeed. Thanks @williamwen42! I left some comments, some require further discussion.

@williamwen42 williamwen42 force-pushed the multiline-traceback-attempt-2 branch from 4094c93 to 425c874 Compare November 21, 2023 22:04
@williamwen42

williamwen42 commented Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

@pablogsal @lysnikolaou can I get a re-review?

@pablogsal

pablogsal commented Nov 27, 2023

Copy link
Copy Markdown
Member

Will make another pass this week. Thanks for your patience @williamwen42!

pablogsal
pablogsal approved these changes Dec 1, 2023
View reviewed changes

@pablogsal pablogsal 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

LGTM

This is a fantastic job @williamwen42 thanks a lot for your patience and for following along with us. You rock 🤘

I left a nit comment but after @lysnikolaou takes a look I think we can land it.

A heads up: it's possible that we may require some changes from now to the first beta if some users or core devs find some aspects of this confusing or not very legible, but we can address those once we have more concrete aspects to discuss.

@pablogsal

pablogsal commented Dec 1, 2023

Copy link
Copy Markdown
Member

@lysnikolaou can you take a look when you have some time?

@lysnikolaou

lysnikolaou commented Dec 1, 2023

Copy link
Copy Markdown
Member

@lysnikolaou can you take a look when you have some time?

On it.

lysnikolaou
lysnikolaou approved these changes Dec 1, 2023
View reviewed changes

@lysnikolaou lysnikolaou 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

Let's merge this! Thanks @williamwen42 for doing this. Great work! 🚀

20 hidden items Load more…
@pablogsal pablogsal merged commit 939fc6d into python:main Dec 1, 2023
@pablogsal

pablogsal commented Dec 1, 2023

Copy link
Copy Markdown
Member

Merged! Congratulations @williamwen42! Thanks a lot for your contribution

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@williamwen42 @aisk
pythongh-106922: Support multi-line error locations in traceback (att… …
72da3a8 
…empt 2) (python#112097)
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@williamwen42 @Glyphack
pythongh-106922: Support multi-line error locations in traceback (att… …
88e3c6c 
…empt 2) (python#112097)
@cretz cretz mentioned this pull request Dec 18, 2024
Merged
picnixz added a commit that referenced this pull request Mar 2, 2025
@picnixz
gh-128481: indicate that the default value for `FrameSummary.end_line… …
c6513f7 
…no` changed in 3.13 (#130755)

The value taken by `FrameSummary.end_lineno` when passing `end_lineno=None` changed in gh-112097.

Previously, a `end_lineno` could be specified to be `None` directly but since 939fc6d, passing None makes
the constructor use the value of `lineno` instead.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 2, 2025
@picnixz @miss-islington
pythongh-128481: indicate that the default value for `FrameSummary.en… …
6fae085 
…d_lineno` changed in 3.13 (pythonGH-130755)

The value taken by `FrameSummary.end_lineno` when passing `end_lineno=None` changed in pythongh-112097.

Previously, a `end_lineno` could be specified to be `None` directly but since 939fc6d, passing None makes
the constructor use the value of `lineno` instead.
(cherry picked from commit c6513f7)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@miss-islington miss-islington mentioned this pull request Mar 2, 2025
Merged
picnixz added a commit that referenced this pull request Mar 2, 2025
@miss-islington @picnixz
[3.13] gh-128481: indicate that the default value for `FrameSummary.e… …
3bef4af 
…nd_lineno` changed in 3.13 (GH-130755) (#130767)

gh-128481: indicate that the default value for `FrameSummary.end_lineno` changed in 3.13 (GH-130755)

The value taken by `FrameSummary.end_lineno` when passing `end_lineno=None` changed in gh-112097.

Previously, a `end_lineno` could be specified to be `None` directly but since 939fc6d, passing None makes
the constructor use the value of `lineno` instead.
(cherry picked from commit c6513f7)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal

@lysnikolaou lysnikolaou lysnikolaou approved these changes

@iritkatriel iritkatriel iritkatriel is a code owner

@isidentical isidentical Awaiting requested review from isidentical

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@williamwen42 @pablogsal @lysnikolaou