Skip to content

Removed conditional import from _collections_abc within collections/_… #5040

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

Merged
merged 3 commits into from
Feb 27, 2021

Conversation

erictraut
Copy link
Contributor

init_.pyi. I don't know why this conditional was added, but it breaks type checking for 3.10 because a bunch of symbols used within this file are not defined.

…_init__.pyi. I don't know why this conditional was added, but it breaks type checking for 3.10 because a bunch of symbols used within this file are not defined.
@hauntsaninja
Copy link
Collaborator

This is to implement the deprecation of using ABCs from collections:

Deprecated since version 3.3, will be removed in version 3.10: Moved Collections Abstract Base Classes to the collections.abc module. For backwards compatibility, they continue to be visible in this module through Python 3.9.

python/cpython@c47c78b#diff-c92f871cb9b4aa1583c89f82cd416d58a5d70fff41faf41e45dedf775dbe7713

@erictraut
Copy link
Contributor Author

erictraut commented Feb 20, 2021

So what is the correct fix? If you don't import these types, there are a bunch of undefined symbols referenced in this file.

The CI tests iterate through different versions of Python, but I suspect that they are not covering Python 3.10 because this would have never passed the CI tests.

@hauntsaninja
Copy link
Collaborator

The correct fix should be to import what is needed without re-exporting. Yes, that's correct — we currently don't run Python 3.10 in CI.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, latest attempt lgtm!

@erictraut
Copy link
Contributor Author

While I think this latest attempt will pass the current CI tests, I suspect it will break as soon as you start running the mypy tests with Python 3.10.

@hauntsaninja
Copy link
Collaborator

Yup. I already opened a PR to fix failures in the stdlib, your test failure conveniently shows what changes need to be made in third party stubs.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

poetry (https://github.com/python-poetry/poetry.git)
+ poetry/utils/helpers.py:26: error: Module 'collections' has no attribute 'Mapping'
+ poetry/utils/helpers.py:26: error: Name 'Mapping' already defined (possibly by an import)

parso (https://github.com/davidhalter/parso.git)
+ parso/python/tree.py:49: error: Module 'collections' has no attribute 'Mapping'
+ parso/python/tree.py:49: error: Name 'Mapping' already defined (possibly by an import)

dulwich (https://github.com/dulwich/dulwich.git)
+ dulwich/config.py:44: error: Module 'collections' has no attribute 'Iterable'
+ dulwich/config.py:44: error: Module 'collections' has no attribute 'MutableMapping'
+ dulwich/config.py:44: error: Name 'Iterable' already defined (possibly by an import)
+ dulwich/config.py:44: error: Name 'MutableMapping' already defined (possibly by an import)


python-htmlgen (https://github.com/srittau/python-htmlgen.git)
+ htmlgen/element.pyi:51: error: Class cannot subclass 'Sized' (has type 'Any')
+ htmlgen/element.pyi:51: error: Name 'collections.Sized' is not defined
+ test_htmlgen/attribute.py:279: error: unused 'type: ignore' comment

@erictraut
Copy link
Contributor Author

@hauntsaninja, I've updated this PR to (I think) fix the incompatibilities that mypy_primer identified. Is there a way to trigger a new run to verify that no more issues are found?

@hauntsaninja
Copy link
Collaborator

Thanks for this!

@hauntsaninja hauntsaninja merged commit 82cb8c2 into python:master Feb 27, 2021
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