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.

Sign up for GitHub

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

Jump to bottom

Add _collections_abc module #4000

Merged
merged 22 commits into from
Jan 23, 2021
Merged

Add _collections_abc module #4000

merged 22 commits into from
Jan 23, 2021

Conversation

CraftSpider
Copy link
Contributor

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

@JelleZijlstra
Copy link
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
Copy link
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
65 tasks
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented May 27, 2020 •
edited
Loading

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
Copy link
Contributor Author

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

@CraftSpider
Copy link
Contributor Author

CraftSpider commented May 28, 2020 •
edited
Loading

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

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

@JukkaL
Copy link
Contributor

JukkaL commented May 29, 2020

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
Copy link
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
Copy link
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
Copy link
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
Copy link
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