-
Notifications
You must be signed in to change notification settings - Fork 23
Do not recommend use of collections.abc.ByteString #350
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
Conversation
It is highly confusing and its isinstance / issubclass behaviour does not match the documentation of typing.ByteString See python/cpython#102092
This comment has been minimized.
This comment has been minimized.
Perhaps we should instead lint against any use of |
tests/imports.pyi
Outdated
@@ -93,7 +93,7 @@ from typing import ContextManager # Y022 Use "contextlib.AbstractContextManager | |||
from typing import OrderedDict # Y022 Use "collections.OrderedDict[KeyType, ValueType]" instead of "typing.OrderedDict[KeyType, ValueType]" (PEP 585 syntax) | |||
from typing_extensions import OrderedDict # Y022 Use "collections.OrderedDict[KeyType, ValueType]" instead of "typing_extensions.OrderedDict[KeyType, ValueType]" (PEP 585 syntax) | |||
from typing import Callable # Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax) | |||
from typing import ByteString # Y022 Use "collections.abc.ByteString" instead of "typing.ByteString" (PEP 585 syntax) | |||
from typing import ByteString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we move this out of the "BAD IMPORTS" section? (It should probably go on line 46)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than my nit, though a short changelog entry might also be in order
Thanks for the review! Added the changelog entry, let me know if you think it's too hostile :D |
This change has no effect on typeshed. 🤖🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me know if you think it's too hostile :D
We're a linter, our entire raison d'être is to shout at people 🔥
Let's hold off on this for now, see the CPython issue. |
What about adding a new code that complains about any usage of ByteString from either typing or collections? That would fit well with the deprecation we're doing at CPython. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we expand this so that collections.abc.ByteString
and typing.ByteString
are both banned? Neither really make sense as annotations in a post-PEP-688 world, and it'll be good to have type checkers linters warn when users use the deprecated types in annotations (especially since stub authors won't see any of the DeprecationWarning
s at runtime, since stubs are never executed at runtime!)
Oh, my review is just saying exactly what @JelleZijlstra said immediately above a few months ago :) Anyway, we should revive this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the above, and this way I can clean up my requested reviews.
@hauntsaninja I'm happy to take this over if you haven't got the time right now :) |
Ah sure, thank you! |
It is highly confusing, its isinstance / issubclass behaviour does not match the documentation of typing.ByteString, and we hope to deprecate it
See python/cpython#102092