Skip to content

bpo-44806: Fix issue with typing.Protocol __init__ method generation - #27541

Closed
uriyyo wants to merge 3 commits into
python:mainfrom
uriyyo:fix-issue-44806
Closed

bpo-44806: Fix issue with typing.Protocol __init__ method generation#27541
uriyyo wants to merge 3 commits into
python:mainfrom
uriyyo:fix-issue-44806

Conversation

@uriyyo

@uriyyouriyyo commented Aug 2, 2021

Copy link
Copy Markdown
Member

@uriyyo

uriyyo commented Aug 2, 2021

Copy link
Copy Markdown
MemberAuthor

@Fidget-Spinner Could you please review this PR?

I curios about why github did not add you as a reviewer? I can see that you at at CODEOWNERS for typing but every time it's add only Guido.

@uriyyo

Copy link
Copy Markdown
MemberAuthor

@serhiy-storchaka Could you please review this PR? I have covered case that you mentioned at a issue tracker.

@Fidget-Spinner

Copy link
Copy Markdown
Member

I curios about why github did not add you as a reviewer? I can see that you at at CODEOWNERS for typing but every time it's add only Guido.

CODEOWNERS only works for people with write access, not for triagers :).

Also sorry, I won't be able to review this PR in time for 3.10rc1. So I'll let Serhiy decide. Both of your PRs look quite similar, just his searches __mro__ and yours searches __bases__.

@ambv

ambv commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

Closing in favor of GH-27545.

@ambvambv closed this Aug 2, 2021
@serhiy-storchaka

Copy link
Copy Markdown
Member

I am sorry, @uriyyo. I started working on a patch at a time with you, and when I finished, I saw your PR, but since my PR covered more cases and contained more tests, I kept it for comparison and discussion. Then I had a problem with connection (resolved only few hours ago), so I did not have opportunity to discuss your PR.

In general, they are almost same. There are subtle differences in corner cases, but for now I cannot say what is more correct.

@uriyyo

Copy link
Copy Markdown
MemberAuthor

@serhiy-storchaka No worries, I am happy that this issue has been resolved🙂

Sign up for freeto 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

@uriyyo@Fidget-Spinner@ambv@serhiy-storchaka@the-knights-who-say-ni@bedevere-bot