Skip to content

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Jul 28, 2021

@uriyyo
Copy link
Member Author

uriyyo commented Jul 28, 2021

@Fidget-Spinner Could you please review this PR?

if '.' in name:
name = name.rpartition('.')[-1]
self.__name__ = name
self.__module__ = _callee(default='typing')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace the body of _callee() with what we currently have in TypeVar.__init__ and use that instead here, in TypeVar, and in ParamSpec? So that all three use cases are identical.

Copy link
Member

Choose a reason for hiding this comment

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

There's a PR refactoring all the uses of this pattern at GH-27387. Though I wasn't too comfortable backporting a refactoring along with a bugfix so I asked him to separate the bugfix into this PR (please let me know if you feel that was wrong).

If you are ok with backporting GH-27387, then we can just merge your suggestions here with that one too.

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

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 28, 2021

Please add the test you described here #27387 (comment)

@uriyyo
Copy link
Member Author

uriyyo commented Jul 28, 2021

@Fidget-Spinner could you please add skip-news label?

@ambv ambv added the skip news label Jul 28, 2021
@uriyyo
Copy link
Member Author

uriyyo commented Jul 28, 2021

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from ambv July 28, 2021 14:14
@uriyyo uriyyo requested a review from Fidget-Spinner July 28, 2021 14:15
@Fidget-Spinner Fidget-Spinner added the needs backport to 3.10 only security fixes label Jul 28, 2021
@ambv
Copy link
Contributor

ambv commented Jul 28, 2021

Wait, what is going on with those pull requests (this one and GH-27387)? I thought the entire point was to make all three types (TypeVar, ParamSpec, and NewType) behave the same. With this patch they don't because the default in callee() should be "__main__", not None, right? This is what BPO-44761 says the PR should be doing.

Also, the test here now is missing any assert statement so it doesn't serve any purpose.

I'll leave comments regarding GH-27387 on that PR.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 28, 2021

With this patch they don't because the default in callee() should be "main", not None, right?

You're right, I missed that. callee() should only return None on implementations without sys._getframe. Otherwise it should return __main__.

Also, the test here now is missing any assert statement so it doesn't serve any purpose.

From what I understand, the test will raise KeyError and fail before this PR. So it has a use.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 28, 2021

Also, the test here now is missing any assert statement so it doesn't serve any purpose.

This test is similar to TypeVar test:

def test_missing__name__(self):
# See bpo-39942
code = ("import typing\n"
"T = typing.TypeVar('T')\n"
)
exec(code, {})

My bad regarding default value. I have fixed it, and now it will return "main" when __name__ is missed. But now _callee has an default parameter that returned when _getframe is missed and it a little bit confusing as for me. Should we rename this parameter or simply return None instead of it?

@ambv
Copy link
Contributor

ambv commented Jul 29, 2021

My bad regarding default value. I have fixed it, and now it will return "main" when __name__ is missed. But now _callee has an default parameter that returned when _getframe is missed and it a little bit confusing as for me. Should we rename this parameter or simply return None instead of it?

You added _callee() just a week ago, this wasn't released yet in any Python version. IMO you should just modify it in this PR to look like it does in GH-27387 to minimize differences between branches.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 29, 2021

Agree, I will copy-paste _callee implementation when we will have concern on how implementation should look like.

@ambv ambv merged commit 7b975f8 into python:main Jul 30, 2021
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-27477 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 30, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 30, 2021
…27406)

(cherry picked from commit 7b975f8)

Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
ambv pushed a commit that referenced this pull request Jul 30, 2021
…GH-27477)

(cherry picked from commit 7b975f8)

Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants