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-106727: Add `__module__` check for `inspect.getsource(cls)` by gaogaotiantian · Pull Request #106968 · python/cpython

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Jul 21, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

In method heuristics, make sure the method we check against comes from the same file as the class. Fixed the issue from #106815

  • Issue: inspect.getsource (findsource) not working as expected when duplicate class is used #106727

Sorry, something went wrong.

Uh oh!

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

All reactions
@gaogaotiantian
Add __module__ check for inspect.getsource
7ff082f
@bedevere-bot bedevere-bot mentioned this pull request Jul 21, 2023
inspect.getsource (findsource) not working as expected when duplicate class is used #106727
Closed
@bedevere-bot bedevere-bot added the awaiting review label Jul 21, 2023
@gaogaotiantian
gaogaotiantian requested a review from carljm July 21, 2023 20:10
@gaogaotiantian gaogaotiantian added the skip news label Jul 21, 2023
@gaogaotiantian

gaogaotiantian commented Jul 21, 2023

Copy link
Copy Markdown
Member Author

This fix is straightforward enough and is a supplement to #106815, so I skipped the news. If @carljm thinks the other way, I can add the news. Also do we want a test for it? It requires some delicate file line number conflict from two files.

carljm 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.

carljm
carljm requested changes Jul 21, 2023
View reviewed changes

@carljm carljm 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.

Can we add a test that fails without this fix?

This doesn't fix the case I mentioned (metaclass and class using it, both with the same name and in the same module), but it does fix the case where the metaclass is in a different module and the line numbers just happen to collide with another class of the same name.

I'm not seeing a clear way to fix the "metaclass with same name" case, and I'm also not really worried about it.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/inspect.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 Jul 21, 2023
@bedevere-bot

bedevere-bot commented Jul 21, 2023

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.

@carljm

carljm commented Jul 21, 2023

Copy link
Copy Markdown
Member

This fix is straightforward enough and is a supplement to #106815, so I skipped the news. If @carljm thinks the other way, I can add the news. Also do we want a test for it? It requires some delicate file line number conflict from two files.

Oops, this comment crossed with my review. I think skipping news is fine, but I think we should have a test, even though it requires additional files. Subtle cases like this will break again in future refactors if there's no test. If it's worth fixing, it's worth testing.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gaogaotiantian
Replaced line breaker with parenthesis, added test case
13f9775
@gaogaotiantian
gaogaotiantian requested a review from carljm July 22, 2023 18:47
@gaogaotiantian

gaogaotiantian commented Jul 22, 2023

Copy link
Copy Markdown
Member Author

Fixed the style and added the test case - used a temporary directory for the files so I don't need to add extra files to the source code. Without the new check, the code would've found the first class as the line number of method borrowed from the other module happens to be inside the first class defined in inspect_actual.

Confirmed that the test would fail without the new check.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm
carljm approved these changes Jul 24, 2023
View reviewed changes
Comment thread Lib/test/test_inspect.py Resolved

Uh oh!

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

Comment thread Lib/test/test_inspect.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jul 24, 2023
@gaogaotiantian @carljm
Remove backslash in test cases
eb2ba7a
Co-authored-by: Carl Meyer <[email protected]>
@carljm
carljm merged commit b383703 into python:main Jul 24, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 24, 2023
jtcave pushed a commit to jtcave/cpython that referenced this pull request Jul 27, 2023
@gaogaotiantian @jtcave
pythongh-106727: Add __module__ check for inspect.getsource(cls) (p…
3801021
…ython#106968)
@Eclips4 Eclips4 mentioned this pull request Jul 29, 2023
test_inspect fails with --forever argument #107446
Closed
@Viicos Viicos mentioned this pull request Aug 10, 2023
Extract attribute docstrings for FieldInfo.description pydantic/pydantic#6563
Merged
5 tasks
@gaogaotiantian
gaogaotiantian deleted the module-check-for-inspect branch September 9, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

3 participants

@gaogaotiantian @bedevere-bot @carljm
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.