Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Python 2 and 3 builtins.pyi #2533

Merged
merged 16 commits into from
Dec 21, 2018
Merged

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Oct 21, 2018

Part of #4

Note to reviewer: It probably makes sense to review the individual commits, especially the first one, since changes are easier to see.

@JukkaL
Copy link
Contributor

JukkaL commented Oct 24, 2018

Looking at the merged stubs, I'm actually not quite sure if this is worth it. There are so many conditional definitions which make the merged stub harder to read, in my opinion. Stubs for builtins are read by users when they want to learn about how things are annotated (e.g. when subclassing a built-in type), so readability has an impact beyond maintainability.

The longer stub will also slow down mypy runtimes slightly, since builtins needs to be always processed. This should only affect the first, non-incremental run, though.

@srittau
Copy link
Collaborator Author

srittau commented Oct 24, 2018

I still find the merged stubs to be well readable. In my opinion it is actually a big advantage that the differences between Python 2 and 3 are easy too see now. Also, I fixed two or three bugs in one version, but not the other while merging, since patches did not get applied to both versions.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks for this Herculean PR! I have two small comments.

I do think the change is worth it. Having a single copy of the stub makes maintenance significantly easier, so I'd almost always prefer a single stub file over two.

stdlib/2and3/builtins.pyi Outdated Show resolved Hide resolved
stdlib/2and3/builtins.pyi Outdated Show resolved Hide resolved
stdlib/2and3/builtins.pyi Outdated Show resolved Hide resolved
@srittau srittau mentioned this pull request Nov 23, 2018
@JelleZijlstra
Copy link
Member

This merge conflicted again. I can merge it once it's fixed.

@srittau
Copy link
Collaborator Author

srittau commented Dec 21, 2018

Done, thanks for looking into this. Considering the amount of PRs we get for builtins, merging it will be helpful. :)

@JelleZijlstra JelleZijlstra merged commit eb1788a into python:master Dec 21, 2018
@srittau srittau deleted the builtins branch December 21, 2018 15:27
msullivan added a commit that referenced this pull request Jan 11, 2019
It looks like it got messed up in #2533
msullivan added a commit that referenced this pull request Jan 11, 2019
It looks like it got messed up in #2533
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
msullivan added a commit that referenced this pull request Feb 8, 2019
This was originally done in #2557, but got lost in #2533.
msullivan added a commit that referenced this pull request Feb 8, 2019
This was originally done in #2557, but got lost in #2533.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants