Skip to content
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

Add basic implementation of PEP657 style expression markers in tracebacks #13102

Merged
merged 7 commits into from
Jan 27, 2025

Conversation

ammaraskar
Copy link
Contributor

Hey folks this is just a proof of concept for #10224 to serve as a reference for if any contributor wants to give this a full shot.

This adds a very basic implementation of the PEP657 style expression highlighting in the short and long traceback modes. It's not the full fledged python traceback.py implementation where we try to differentiate which parts contain operators.

I don't think I'll have time to finish this out fully, there's some interesting problems here like mypy complaining because typeshed doesn't have some of the newer traceback FrameSummary attrs like colno. Additionally, since the python offsets are from the start of the line, we have to know the line before it gets de-dented in Source.


Example of what it looks like:

image

and with tb short:

image

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

I don't think I'll have time to finish this out fully

Thanks for taking it this far! Whether or not you keep going, this is a neat contribution 😄

There's some interesting problems here like mypy complaining because typeshed doesn't have some of the newer traceback.FrameSummary attrs like colno.

It looks like typeshed does have them, so I think we're probably seeing mypy pointing out that we need to version-guard those accesses; see comment below.

Additionally, since the python offsets are from the start of the line, we have to know the line before it gets de-dented in Source.

I think we could fetch it with linecache (as used by the traceback module), and then correct for the different length of leading whitespace? Alternatively we could ast.parse() the Source line, and see if there's an expression which matches the expected start and end colnos for various candidate dedents; it's a heuristic but likely to be unambiguous almost all the time.

Comment on lines +876 to +877
assert reprtb.lines[0] == " return 1 / 0"
assert reprtb.lines[1] == " ^^^^^"
Copy link
Member

Choose a reason for hiding this comment

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

It's not the full fledged python traceback.py implementation where we try to differentiate which parts contain operators.

That would be very nice to have, but I'd be happy to merge something without that and open a follow-up issue: this is already a nice improvement on the status quo!

@ammaraskar
Copy link
Contributor Author

ammaraskar commented Jan 4, 2025

Oh yup, you're totally right about them being behind a version check in typeshed, I missed that. Added a version guard and now mypy seems happy with it.

Also added version guards for the one unit test I added and changed.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Thanks again @ammaraskar!

I think the only thing left is to add yourself to the AUTHORS file, and create changelog/10224.improvement.rst containing something like "Partial support for PEP-657 precise traceback locations in Pytest's alternative traceback formatting."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jan 26, 2025
@ammaraskar ammaraskar marked this pull request as ready for review January 26, 2025 20:32
@ammaraskar
Copy link
Contributor Author

Added a changelog and AUTHORS entry.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines 236 to 237
if frame_summary.end_lineno is None:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looks like we need tests covering this branch - though I'm not sure there's a nice way to get that, so maybe a # pragma: no cover instead? Hmm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it would be tough to disable the column information to test this branch. Added a no cover.

@ammaraskar ammaraskar changed the title Draft: Add very basic implementation of PEP657 style expression markers in tracebacks Add basic implementation of PEP657 style expression markers in tracebacks Jan 27, 2025
@Zac-HD Zac-HD merged commit a0e3a49 into pytest-dev:main Jan 27, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants