Skip to content

Add _collections_abc module - #4000

Merged
srittau merged 22 commits into
python:masterfrom
CraftSpider:collections-stubs
Jan 23, 2021
Merged

Add _collections_abc module#4000
srittau merged 22 commits into
python:masterfrom
CraftSpider:collections-stubs

Conversation

@CraftSpider

@CraftSpider CraftSpider commented May 16, 2020

Copy link
Copy Markdown
Contributor

I'm quite amused: The way typeshed does this, re-exporting from typing, is the reverse of how runtime does it

@JelleZijlstra

JelleZijlstra commented May 16, 2020

Copy link
Copy Markdown
Member

Should we put the implementations here and make collections.abc import * from here? That more closely matches what runtime does but I'm not sure it makes a practical difference.

@CraftSpider

CraftSpider commented May 16, 2020

Copy link
Copy Markdown
Contributor Author

Personally, I'd like to do that to match runtime, but I don't think it matters either way

@CraftSpider CraftSpider mentioned this pull request May 16, 2020
Closed
61 of 65 tasks
hauntsaninja
hauntsaninja reviewed May 16, 2020
View reviewed changes
Comment thread stdlib/3/_collections_abc.pyi Outdated
hauntsaninja
hauntsaninja reviewed May 16, 2020
View reviewed changes
Comment thread stdlib/3/_collections_abc.pyi Outdated
hauntsaninja
hauntsaninja reviewed May 16, 2020
View reviewed changes
Comment thread stdlib/3/collections/__init__.pyi Outdated
@hauntsaninja

hauntsaninja commented May 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Oh, right, this was a little mysterious to me, and seems to still be failing. The changes look fairly straightforward, so not sure what's going on. If I had to take a guess — I've seen some weird behaviour surrounding use of __all__ in stubs and this could definitely be __all__-related; maybe try removing all mentions of __all__?
Alternatively, maybe someone else knows for sure what's wrong.

@CraftSpider

CraftSpider commented May 27, 2020

Copy link
Copy Markdown
Contributor Author

See my reply above, it is because of __all__. Thank you for the help

@CraftSpider

CraftSpider commented May 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Playing around with pytype, I think it fixes the issue to add MutableMapping = MutableMapping and etc for every type after the import in _collections_abc. Is this my fault, or is pytype choking on the re-exports? (ping @rchen152)

JelleZijlstra
JelleZijlstra approved these changes May 28, 2020
View reviewed changes

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

This looks correct apart from the CI failure and merge conflict.

@JukkaL

JukkaL commented May 29, 2020

Copy link
Copy Markdown
Contributor

Should we put the implementations here and make collections.abc import * from here?

I think that this might result in mypy showing _collections_abc in some error messages, so the way these are done currently is arguably a bit better for mypy usability. Also, the current structure is probably more convenient for somebody who just wants to look up how something is defined in the stubs, as they won't have to dig deeper into internal modules.

@rchen152

rchen152 commented Sep 14, 2020

Copy link
Copy Markdown
Collaborator

Sorry for the belated response - we noticed today that pytype seems to choke on re-importing a name without changing it (so from x import y as z works, but from x import y as y does not), so the CI issue here seems like a pytype bug. Hopefully will be fixed shortly.

rchen152 added a commit to google/pytype that referenced this pull request Sep 15, 2020
@rchen152
Don't crash when processing a pyi containing __all__.
c44a38d 
Other type checkers rely on the actual value of __all__, so it is sometimes
included in pyi files. pytype was crashing due to not knowing how to process an
alias whose type was a literal list.

Discovered while investigating why pytype failed on
python/typeshed#4000.

PiperOrigin-RevId: 331886378
rchen152 added a commit to google/pytype that referenced this pull request Sep 16, 2020
@rchen152
Don't crash when processing a pyi containing __all__.
f71834c 
Other type checkers rely on the actual value of __all__, so it is sometimes
included in pyi files. pytype was crashing due to not knowing how to process an
alias whose type was a literal list.

Discovered while investigating why pytype failed on
python/typeshed#4000.

PiperOrigin-RevId: 331886378
rchen152 added a commit to google/pytype that referenced this pull request Sep 17, 2020
@rchen152
Respect all typing reexports in the pyi parser.
917e9b1 
Previously, we respected 'from typing import X as Y' but ignored
'from typing import X as X', with the effect that a second pyi that tried to
import the re-exported name wouldn't parse due to a BadDependencyError.

Found while investigating python/typeshed#4000.

PiperOrigin-RevId: 332290196
@rchen152

rchen152 commented Sep 25, 2020

Copy link
Copy Markdown
Collaborator

It's a little hard to tell since the PR doesn't merge cleanly anymore, but I believe pytype 2020.09.24 fixes the pytype test failure. Let me know if you're still having issues.

@JelleZijlstra

JelleZijlstra commented Nov 1, 2020

Copy link
Copy Markdown
Member

This has some pretty bad merge conflicts now, @CraftSpider do you want to fix them? I think this should be good to go once CI is green.

@CraftSpider

CraftSpider commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

I'll fix the merge conflicts as soon as I have the free time to

@srittau
srittau merged commit 6aa5cc6 into python:master Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

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

@CraftSpider @JelleZijlstra @hauntsaninja @JukkaL @rchen152 @srittau