gh-91389: Fix show_caches in dis module#32406
Conversation
the-knights-who-say-ni commented Apr 7, 2022
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
brandtbucher left a comment
brandtbucher left a comment There was a problem hiding this comment.
Thanks! A few notes:
- Please sign the CLA (you've probably seen the bot's message by now).
- This needs a NEWS entry. Something simple like "
Fix an issue where passing ``show_caches=True`` to :mod:`dis` utilities could result in incorrect position information." should be fine. - It also needs at least one regression test in
Lib/test/test_dis.pythat fails before this change and passes after. - You can also add your name to
Misc/ACKS, if you want. 😎
Let me know if you have any questions.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Apr 7, 2022
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
15r10nk commented Apr 7, 2022
I have made the requested changes; please review again |
bedevere-bot commented Apr 7, 2022
Thanks for making the requested changes! @brandtbucher: please review the changes made to this pull request. |
15r10nk commented Apr 8, 2022
Please don't merge. I want to improve the unit test. |
15r10nkforce-pushed the fix-issue-47233 branch 2 times, most recently from 963afcc to 3fd5c3eCompare15r10nk commented Apr 10, 2022
I changed the unit test. What do you think @brandtbucher ? |
brandtbucher left a comment
brandtbucher left a comment There was a problem hiding this comment.
Almost there... just a couple of naming/formatting improvements:
Lib/test/test_dis.py Outdated
There was a problem hiding this comment.
| defgetInstructions(show_caches): | |
| defget_instructions(show_caches): |
Lib/test/test_dis.py Outdated
There was a problem hiding this comment.
| self.assertEqual(getInstructions(True),getInstructions(False)) | |
| self.assertEqual(get_instructions(True),get_instructions(False)) |
Lib/test/test_dis.py Outdated
There was a problem hiding this comment.
| a.b.c(1*x) | |
| ifx: | |
| c=5 | |
| a.b.c(1*x) | |
| ifx: | |
| c=5 |
bedevere-bot commented Apr 13, 2022
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
The following commit authors need to sign the Contributor License Agreement: Click the button to sign: |
15r10nk commented Apr 14, 2022
I have made the requested changes; please review again |
bedevere-bot commented Apr 14, 2022
Thanks for making the requested changes! @brandtbucher: please review the changes made to this pull request. |
brandtbucher changed the title 
ambv commented Apr 14, 2022
@15r10nk, can you re-sign the CLA using your polarbit.de address? If the CLA bot doesn't let you, please add this email to your Github emails in settings. If you click on the list of commits in this PR now, you will see that the author of the commit is listed as " Frank Hoffmann" and doesn't link back to your profile. |
| starts_line=None | ||
| foroffset, op, argin_unpack_opargs(code): | ||
| # get the next position before we skip a CACHE instruction | ||
| # to associate the correct position information. |
There was a problem hiding this comment.
Also, the CI is mad about this trailing whitespace. Didn't see it until now:
| # to associate the correct position information. | |
| # to associate the correct position information. |
15r10nk commented Apr 14, 2022
Yes, I will do, unfortunately not before next week Wednesday. |
15r10nk commented Apr 20, 2022
It seems that I was to late :-). |
5 participants
https://bugs.python.org/issue47233