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.

Sign up for GitHub

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

Jump to bottom

gh-91389: Fix show_caches in dis module #32406

Closed
wants to merge 1 commit into from
Closed

gh-91389: Fix show_caches in dis module #32406

wants to merge 1 commit into from

Conversation

15r10nk
Copy link
Contributor

@15r10nk 15r10nk commented Apr 7, 2022 •
edited by bedevere-bot
Loading

@the-knights-who-say-ni
Copy link

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 Missing

Our records indicate the following people have not signed the CLA:

@15r10nk

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@brandtbucher brandtbucher self-requested a review April 7, 2022 21:01
brandtbucher
brandtbucher requested changes Apr 7, 2022
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

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.py that 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.

Lib/dis.py Show resolved Hide resolved
@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 7, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 8, 2022

Please don't merge. I want to improve the unit test.

@15r10nk 15r10nk force-pushed the fix-issue-47233 branch 2 times, most recently from 963afcc to 3fd5c3e Compare April 8, 2022 19:35
@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 10, 2022

I changed the unit test. What do you think @brandtbucher ?

brandtbucher
brandtbucher requested changes Apr 13, 2022
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Almost there... just a couple of naming/formatting improvements:

Lib/test/test_dis.py Outdated
if x:
c=5

def getInstructions(show_caches):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def getInstructions(show_caches):
def get_instructions(show_caches):

Lib/test/test_dis.py Outdated
if instr.opname != "CACHE"
]

self.assertEqual(getInstructions(True),getInstructions(False))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEqual(getInstructions(True),getInstructions(False))
self.assertEqual(get_instructions(True), get_instructions(False))

Lib/test/test_dis.py Outdated
Comment on lines 1625 to 1627
a.b.c(1*x)
if x:
c=5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a.b.c(1*x)
if x:
c=5
a.b.c(1 * x)
if x:
c = 5

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 14, 2022 •
edited
Loading

The following commit authors need to sign the Contributor License Agreement:

Click the button to sign:
CLA not signed

@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 14, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@brandtbucher brandtbucher changed the title bpo-47233: Fix show_caches in dis module gh-91389: Fix show_caches in dis module Apr 14, 2022
@ambv ambv removed the CLA signed label Apr 14, 2022
@ambv
Copy link
Contributor

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.

brandtbucher
brandtbucher reviewed Apr 14, 2022
View reviewed changes
Lib/dis.py
@@ -413,6 +413,9 @@ def _get_instructions_bytes(code, varname_from_oparg=None,
labels.add(target)
starts_line = None
for offset, op, arg in _unpack_opargs(code):
# get the next position before we skip a CACHE instruction
# to associate the correct position information.
Copy link
Member

Choose a reason for hiding this comment

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

Also, the CI is mad about this trailing whitespace. Didn't see it until now:

Suggested change
# to associate the correct position information.
# to associate the correct position information.

@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 14, 2022

Yes, I will do, unfortunately not before next week Wednesday.

@ambv ambv closed this Apr 15, 2022
@ambv ambv reopened this Apr 15, 2022
@ambv ambv closed this Apr 15, 2022
@ambv ambv reopened this Apr 15, 2022
@ambv ambv closed this Apr 15, 2022
@ambv ambv reopened this Apr 15, 2022
@15r10nk
Copy link
Contributor Author

15r10nk commented Apr 20, 2022

It seems that I was to late :-).
The problem was fixed with e590379.

@15r10nk 15r10nk mannequin mentioned this pull request Apr 20, 2022
Closed
@15r10nk 15r10nk closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandtbucher brandtbucher brandtbucher requested changes

Assignees
No one assigned
Labels
awaiting change review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@15r10nk @the-knights-who-say-ni @bedevere-bot @ambv @brandtbucher