-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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-106922: Support multi-line error locations in traceback #109589
Conversation
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Thanks for the PR @williamwen42! I will review this shortly but two initial comments:
|
I am also a bit concerned over how much C code the required implementation requires. I need to review it but my initial impression is that this may be a bit too much. What do others think? @isidentical @ammaraskar @iritkatriel |
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
@pablogsal any idea when you can take a look at this PR? There are some failing tests but those should just be small fixes that don't have a large impact on the PR. LMK if you would like an even more detailed description of the changes. |
@williamwen42 We have refactored the whole traceback machinery so CPython uses mainly the traceback module (in Python) and falls back to the C implementation but we ripped out all the error locations in the C implementation so is easier to maintain. This means that we can remove the C parts of this PRs. You may have some conflicts on the Python part yet, but they shouldn't be too big. I know you probably spent a lot of time doing the C part, but this change was involving a lot of C code that we woudn't be able to merge because the complexity outweighs what we are gaining from it, but given that now is only on the Python side, then we can consider it. |
CC: @lysnikolaou |
Since 'attempt 2' is merged, close this? |
Complete implementation for #106922. Implementing (1) requires substantial changes for an implementation of (2) or (3), so I just did it all in one go.
Some changes compared to the initial issue:
We do not always output all lines that correspond to an error:
test_many_lines_binary_op
).Order of looking things in the PR:
FrameSummary
fieldsformat_frame_summary
and_extract_caret_anchors_from_line_segment
- comments should sufficiently explain the algorithmtraceback.py
- comments should sufficiently explain the algorithmdedent
and for tracking significant lines (struct SignificantLines
i.e. a set). If there are existing implementations, then I would be happy to use those instead.cc @pablogsal @isidentical @ammaraskar @iritkatriel @ezyang