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

bpo-39114: Fix tracing of except handlers with name binding #17769

Open
wants to merge 1 commit into
base: master
from

Conversation

@pablogsal
Copy link
Member

pablogsal commented Dec 31, 2019

@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Dec 31, 2019

@nedbat Can you confirm if this fixes your issue?

@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Dec 31, 2019

@markshannon Can you check if this looks right to you? I am not completely sure what was the original purpose of this code (not sure what the comment Make sure that we trace line after exception is referring to) but looks like this is what is making the trace function emit incorrect lines after the artificial finally of the try-except with name to delete de name.

@nedbat

This comment has been minimized.

Copy link
Member

nedbat commented Dec 31, 2019

@pablogsal Thanks, I'll take a look at it. Right now, this change is failing other tests of mine. We should talk about how to add more regression tests to the Python test suite so that we don't have to have these kinds of bug reports in the first place.

As an example, shouldn't this pull request add some tests to confirm the fix?

@nedbat

This comment has been minimized.

Copy link
Member

nedbat commented Dec 31, 2019

@pablogsal good news: looks like the coverage.py tests that fail with this change are testing the same things that the current test failure here is testing.

@pablogsal

This comment has been minimized.

Copy link
Member Author

pablogsal commented Dec 31, 2019

As an example, shouldn't this pull request add some tests to confirm the fix?

Yes, but for now I am not sure that the fix is even correct so I still didn't make tests for it :)

@pablogsal pablogsal force-pushed the pablogsal:fix branch from 8b8f4a7 to 7617a9d Dec 31, 2019
rsumner868 added a commit to rsumner868/master that referenced this pull request Dec 31, 2019
@pablogsal pablogsal force-pushed the pablogsal:fix branch from 7617a9d to 1edfb0a Dec 31, 2019
@pablogsal pablogsal removed the DO-NOT-MERGE label Dec 31, 2019
@pablogsal pablogsal force-pushed the pablogsal:fix branch from 1edfb0a to 70be868 Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.