Skip to content

gh-125593: Use colors to highlight error locations in tracebacks from exception group #125681

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

Merged
merged 9 commits into from
Oct 27, 2024

Conversation

wrongnull
Copy link
Contributor

@wrongnull wrongnull commented Oct 18, 2024

Comment on lines 4658 to 4659
self.assertIn(f"{red}1 {reset+boldr}/{reset+red} 0{reset}", actual)
self.assertIn(f"{red}~~{reset+boldr}^{reset+red}~~{reset}", actual)
Copy link
Member

@Eclips4 Eclips4 Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to test for the presence of the magenta color, and, more importantly, we need to ensure that the exception group has been colored and that their sub-exception have also been colored.

@Eclips4 Eclips4 requested a review from pablogsal October 18, 2024 07:36
@Eclips4
Copy link
Member

Eclips4 commented Oct 18, 2024

@pablogsal @Yhg1s I think this is on the boundary between a bug fix and a feature, so I wanted to ask for your opinion on backporting it to the 3.13 branch. This pull request is pretty trivial, so I would like to consider it a bug fix.

@pablogsal
Copy link
Member

pablogsal commented Oct 18, 2024

It's 100% bug, colors should apply to all exceptions, not just normal ones (as it's documented).

@Yhg1s
Copy link
Member

Yhg1s commented Oct 18, 2024

Yes, treating this as a bug seems correct, given that the traceback is now partially colourised.

@wrongnull
Copy link
Contributor Author

I'm converting it to a draft while I struggle with test debugging.

@wrongnull wrongnull marked this pull request as draft October 18, 2024 14:24
@Eclips4 Eclips4 added the needs backport to 3.13 bugs and security fixes label Oct 18, 2024
@wrongnull wrongnull requested a review from Eclips4 October 18, 2024 15:04
@wrongnull wrongnull marked this pull request as ready for review October 18, 2024 15:04
@wrongnull
Copy link
Contributor Author

I definitely underestimated myself

@wrongnull
Copy link
Contributor Author

We can't just construct a expected string and compare it's to the actual message from traceback due to that fact that on Windows x86 <object at 0x...> can't be constructed using the f"<object at {hex(id(obj))}>" because it has different formatting for pointers...:

First differing element 5:
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x06176570>'
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x6176570>'

@pablogsal What do you think? Is the current approach looks good to you or it need to be more generic?

@pablogsal
Copy link
Member

We can't just construct a expected string and compare it's to the actual message from traceback due to that fact that on Windows x86 <object at 0x...> can't be constructed using the f"<object at {hex(id(obj))}>" because it has different formatting for pointers...:

First differing element 5:
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x06176570>'
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x6176570>'

@pablogsal What do you think? Is the current approach looks good to you or it need to be more generic?

You could use the real foo object in the f-string so it's formatted directly in the expectation no? That will respect whatever custom formatting is done per platform

@wrongnull
Copy link
Contributor Author

You could use the real foo object in the f-string so it's formatted directly in the expectation no? That will respect whatever custom formatting is done per platform

I can't. Since pointer formatting is implementation-defined behavior, 32-bit MSVC omits leading zero in formatted string

@wrongnull
Copy link
Contributor Author

We can't just construct a expected string and compare it's to the actual message from traceback due to that fact that on Windows x86 <object at 0x...> can't be constructed using the f"<object at {hex(id(obj))}>" because it has different formatting for pointers...:

First differing element 5:
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x06176570>'
'  | [54 chars]zed_traceback_from_exception_group.<locals>.foo at 0x6176570>'

@pablogsal What do you think? Is the current approach looks good to you or it need to be more generic?

You could use the real foo object in the f-string so it's formatted directly in the expectation no? That will respect whatever custom formatting is done per platform

I apologize for the misunderstanding. Appropriate changes have been made.

Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@pablogsal what do you think about the current approach to testing it?

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pablogsal pablogsal merged commit 51b012b into python:main Oct 27, 2024
37 checks passed
@miss-islington-app
Copy link

Thanks @wrongnull for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@pablogsal
Copy link
Member

Thanks a lot for the fix @wrongnull and thanks for the review @Eclips4 !

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2024
…s from exception group (pythonGH-125681)

(cherry picked from commit 51b012b)

Co-authored-by: Bogdan Romanyuk <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2024

GH-126021 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 27, 2024
pablogsal pushed a commit that referenced this pull request Oct 27, 2024
…ks from exception group (GH-125681) (#126021)

gh-125593: Use colors to highlight error locations in tracebacks from exception group (GH-125681)
(cherry picked from commit 51b012b)

Co-authored-by: Bogdan Romanyuk <[email protected]>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants