GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91389: Fix show_caches in dis module by 15r10nk · Pull Request #32406 · python/cpython

Conversation

@15r10nk

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue47233

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the awaiting review label Apr 7, 2022
@the-knights-who-say-ni

the-knights-who-say-ni commented Apr 7, 2022

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Apr 7, 2022
@brandtbucher
brandtbucher self-requested a review April 7, 2022 21:01
brandtbucher
brandtbucher requested changes Apr 7, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/dis.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Apr 7, 2022
@bedevere-bot

bedevere-bot commented Apr 7, 2022

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@15r10nk
15r10nk force-pushed the fix-issue-47233 branch from 2f30bd2 to 9e37f18 Compare April 7, 2022 21:57
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Apr 7, 2022
@15r10nk

15r10nk commented Apr 7, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 7, 2022
@bedevere-bot

bedevere-bot commented Apr 7, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from brandtbucher April 7, 2022 22:01
@15r10nk

15r10nk commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

Please don't merge. I want to improve the unit test.
brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@15r10nk
15r10nk force-pushed the fix-issue-47233 branch 2 times, most recently from 963afcc to 3fd5c3e Compare April 8, 2022 19:35
@15r10nk

15r10nk commented Apr 10, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher
brandtbucher requested changes Apr 13, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_dis.py Outdated

@brandtbucher brandtbucher Apr 13, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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):

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_dis.py Outdated

@brandtbucher brandtbucher Apr 13, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_dis.py Outdated
Comment on lines 1625 to 1627

@brandtbucher brandtbucher Apr 13, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels Apr 13, 2022
@bedevere-bot

bedevere-bot commented Apr 13, 2022

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@15r10nk
bpo-47233: Fix show_caches in dis module
82e0ecb
@15r10nk
15r10nk force-pushed the fix-issue-47233 branch from 3fd5c3e to 82e0ecb Compare April 14, 2022 07:22
@ghost

ghost commented Apr 14, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

  • [email protected]

Click the button to sign:
CLA not signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@15r10nk

15r10nk commented Apr 14, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 14, 2022
@bedevere-bot

bedevere-bot commented Apr 14, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from brandtbucher April 14, 2022 07:31
@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

ambv commented Apr 14, 2022

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher
brandtbucher reviewed Apr 14, 2022
View reviewed changes
Comment thread Lib/dis.py
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.

@brandtbucher brandtbucher Apr 14, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@15r10nk

15r10nk commented Apr 14, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

15r10nk commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@15r10nk 15r10nk mannequin mentioned this pull request Apr 20, 2022
show_caches option affects code positions reported by dis.get_instructions(...) #91389
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting change review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@15r10nk @the-knights-who-say-ni @bedevere-bot @ambv @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.