FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-39942:Fix `TypeVar` fails when missing `__name__` by hongweipeng · Pull Request #19616 · python/cpython · GitHub

/ cpython Public

bpo-39942:Fix TypeVar fails when missing __name__ - #19616

Merged
ilevkivskyi merged 2 commits into
python:masterfrom
hongweipeng:issue39942
Apr 20, 2020
ConversationCommits (2)ChecksFiles changed
Merged

bpo-39942:Fix TypeVar fails when missing __name__#19616
ilevkivskyi merged 2 commits into
python:masterfrom
hongweipeng:issue39942

Conversation

hongweipeng commented Apr 20, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

remilapeyre reviewed Apr 20, 2020
View reviewed changes

remilapeyre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for opening a pull request @hongweipeng, I tested the changes and it works well, I'm not sure if defaulting to __main__ is the correct behavior thought. If I'm correct both Enums and dataclasses had this issue and they are now just unpickable when __name__ cannot be determined.

Comment thread Lib/typing.py
if def_mod != 'typing':
self.__module__ = def_mod
try:
def_mod = sys._getframe(1).f_globals.get('__name__', '__main__') # for pickling

remilapeyre Apr 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think it's safe to assume __name__ is __main__ when it's not defined here. I think other classes like enums make the class unpickable when __name__ cannot be determined.

ilevkivskyi Apr 20, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think '__main__' is fine. By the way a similar code is used by TypedDict and NamedTuple, so you can copy it here. Or maybe it would even make sense to factor this out in an internal helper function doing this:

    try:
        module = sys._getframe(1).f_globals.get('__name__', '__main__')
    except (AttributeError, ValueError):
        module = None

Comment thread Lib/test/test_typing.py
with self.assertRaises(TypeError):
TypeVar('X', str, float, bound=Employee)

def test_missing__name__(self):

remilapeyre Apr 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
    def test_missing__name__(self):
        # See bpo-39942
        code = ("import typing\n"
                "T = typing.TypeVar('T')\n"
                )
        exec(code, {})

would be more like the other tests

remilapeyre commented Apr 20, 2020

Copy link
Copy Markdown
Contributor

This change will also require a NEWS entry (https://devguide.python.org/committing/#what-s-new-and-news-entries), you can add one by using https://blurb-it.herokuapp.com/

ilevkivskyi approved these changes Apr 20, 2020
View reviewed changes

ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good, thanks!

ilevkivskyi merged commit a25a04f into python:master Apr 20, 2020

ilevkivskyi commented Apr 20, 2020

Copy link
Copy Markdown
Member

Actually, it probably makes sense to backport this to 3.7 and 3.8 since this is technically a bug. Let me try if this still can be done automatically.

miss-islington commented Apr 20, 2020

Copy link
Copy Markdown
Contributor

Thanks @hongweipeng for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington commented Apr 20, 2020

Copy link
Copy Markdown
Contributor

Thanks @hongweipeng for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 20, 2020
bpo-39942:Fix failure in TypeVar when missing __name__ (pythonGH-…
ee285cd 
…19616)

https://bugs.python.org/issue39942
(cherry picked from commit a25a04f)

Co-authored-by: HongWeipeng <hongweichen8888@sina.com>

bedevere-bot commented Apr 20, 2020

Copy link
Copy Markdown

GH-19626 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 20, 2020
bpo-39942:Fix failure in TypeVar when missing __name__ (pythonGH-…
15f745b 
…19616)

https://bugs.python.org/issue39942
(cherry picked from commit a25a04f)

Co-authored-by: HongWeipeng <hongweichen8888@sina.com>

bedevere-bot commented Apr 20, 2020

Copy link
Copy Markdown

GH-19627 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Apr 20, 2020
bpo-39942:Fix failure in TypeVar when missing __name__ (GH-19616)
694a95f 
https://bugs.python.org/issue39942
(cherry picked from commit a25a04f)

Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
miss-islington added a commit that referenced this pull request Apr 20, 2020
bpo-39942:Fix failure in TypeVar when missing __name__ (GH-19616)
41660ca 
https://bugs.python.org/issue39942
(cherry picked from commit a25a04f)

Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
hongweipeng deleted the issue39942 branch April 21, 2020 01:10
EvanKirshenbaum mentioned this pull request Jan 30, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ilevkivskyi ilevkivskyi approved these changes
gvanrossum Awaiting requested review from gvanrossum
+1 more reviewer
remilapeyre remilapeyre left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL