-
Notifications
You must be signed in to change notification settings - Fork 334
Imports refactoring #1815
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
base: develop
Are you sure you want to change the base?
Imports refactoring #1815
Conversation
dfa1a49 to
ad21cb6
Compare
Changed ------- - `build_and_test.yml` workflow_dispatch trigger added - Updated `typing_extensions` dependency platform flags - Simplified conditional imports from `typing` with alternatives from `typing_extensions` that resolves it under the hood, so we need no control it manually - Legacy `collections.abc` imports replaced with `typing_extensions` cause current required `typing_extensions>=4.7` version already supports all them - Removed redundant (legacy) imports from `__future__` - Undefined annotations fixed Added ----- - Added `_compat.py` module to simplify future support for other python implementations, like `pypy` or `circuitpython`
ad21cb6 to
952f579
Compare
dlech
left a comment
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.
If there was some new typing feature that we would need typing_extensions for in more recent versions of Python, I could be convinced to not make it a conditional dependency. But as it is, I think this PR is too heavy-handed in undoing things that were intentionally done (see inline comments).
bleak/__init__.py
Outdated
| import os | ||
| import sys | ||
| import uuid | ||
| from collections.abc import AsyncGenerator, Awaitable, Callable, Iterable |
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.
collections.abc is the canonical place to import these from since Python 3.9, so this feels like a step backwards as well.
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.
Agree, but the problem this types are defined in different places for each python versions starting from 3.9, so if we know typing_extensions are duarantee reimport it as needed for each version why should we do it by ourself?
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.
Till old python versions support will be done it looks quite good to use backward compatibility through typing_extensions instead of conditional imports.
Besides, I have great doubts that you will completely abandon typing_extension in the next few years.
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.
We have already dropped support for Python < 3.9, so I don't see why we would need to change this. typing_extensions just forwards typing declarations of these types and these types are deprecated in the typing module.
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.
That’s why it’s logical. Instead of doing it manually in each individual module, you use a tool that does it under the hood, allowing you to focus on implementation rather than annotation compatibility. Why are you so sceptical to typing_extensions?
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.
rolled back
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.
Why are you so sceptical to
typing_extensions?
Not skeptical. One of the original core design decisions of Bleak was minimal dependencies. So that has been what has guided the current state of things.
pyproject.toml
Outdated
| [tool.poetry.dependencies] | ||
| async-timeout = { version = ">=3.0.0", python = "<3.11" } | ||
| typing-extensions = { version = ">=4.7.0", python = "<3.12" } | ||
| typing-extensions = { version = ">=4.7.0" } |
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.
Ideally, we would drop the typing-extensions dependency in a few years when we drop support for Python 3.11. This is why we have the extra conditionals in the code and here.
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.
That is, but if you wanna conditional imports it would be a good practice then place it inside _compat.py module. You have such much this along the codebase
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.
Till 3.11 will be dropped I propose use typing_extensions directly, that is almost affects nothing except readability, but makes maintenance and extending simplier
If it's that fundamental, then the general practice is: "It's easier to ask for forgiveness than permission" so instead of |
|
How can you explain that you mix your own version-based resolving with forwards from typing_extensions? Why don't you rely entirely on a self-written implementation then? |
17192c7 to
a744a60
Compare
|
|
|
||
| # prevent tasks from being garbage collected | ||
| _background_tasks = set[asyncio.Task[None]]() | ||
| _background_tasks: set[asyncio.Task[None]] = set() |
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.
Same as in init
| .. versionadded:: 0.21 | ||
| """ | ||
| devices = asyncio.Queue[tuple[BLEDevice, AdvertisementData]]() | ||
| devices: asyncio.Queue[tuple[BLEDevice, AdvertisementData]] = asyncio.Queue() |
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.
Same as previous
|
@dlech |
|
@dlech
|
|
So, regarding the drama surrounding PEP563 and Guido's decision at the time, |
This PR is solving #1813 issue and preparing project to resolve #1812
Changed
build_and_test.ymlworkflow_dispatch trigger addedUpdatedtyping_extensionsdependency platform flagstypingwith alternatives fromtyping_extensionsthat resolves it under the hood, so we need no control it manually
Legacycollections.abcimports replaced withtyping_extensionscause current required
typing_extensions>=4.7version already supports all themRemoved redundant (legacy) imports from__future__Added
_compat.pymodule to simplify future support for other python implementations, likepypyorcircuitpython