-
Notifications
You must be signed in to change notification settings - Fork 334
Centralize backend detection #1840
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
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.
As a suggestion, we could take this one step farther. Instead of creating an enum class, we could create a BackendProvider class that has two attributes, the scanner_type and client_type. Then we could remove the two get_platform_{client,scanner}_backend_type() functions completely and just use the new get_backend() (or get_backend_provider() function to replace that.
That is, replace:
PlatformBleakClient = (
get_platform_client_backend_type() if backend is None else backend
)with
PlatformBleakClient = (
get_backend_provider().client_type if backend is None else backend
)
bleak/backends/__init__.py
Outdated
| P4Android = enum.auto() | ||
| BlueZDBus = enum.auto() | ||
| PythonistaCB = enum.auto() | ||
| CoreBluetooth = enum.auto() |
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.
I would be tempted to call this one PyObjCCB since we are looking at adding a Rubicon ObjC backend as well.
It is kind of a special case since we would have two backends sharing code, so maybe you had something else in mind?
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.
I would differentiate bleak backends and the binding frameworks. The bleak backend is CoreBluetooth, the binding frameworks for this backend are pyobjc or rubicon-objc.
In my currently local testing for rubicon-objc the most code for CoreBluetooth is shared. So I don’t think that it should be listed as a complete new backend. The detection logic which binding framework is used can be inside the CoreBluetooth backend itself.
Not sure how clear I was on this and it was pretty simple anyway, so I went ahead and tried it myself in #1845. |
So, your PR #1845 is providing only a solution for the code deduplication. But my second intention of this PR was to provide an |
Ah-ha. That could be useful as well at runtime to help handle platform-specific quirks. |
1ca33bd to
9200840
Compare
|
I still have a small hesitation before merging this. There are some 3rd-party backends for Bleak. These get passed into the Up to now, this is the recommended way to get the actual backend: Line 29 in e68cbd8
|
|
To use I implemented the following changes:
|
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.
Thanks for the update. I'm happy with the changes. Just a few more minor suggestions before we merge.
|
#1855 should fix the failing tests here. |
|
I added a bit to make the new stuff actually show up in the docs. Ready to merge now, thanks! |
Currently the backend detection logic is duplicated in
bleak.backends.clientandbleak.backends.scanner. This PR centralizes the backend detection and added a newBleakBackendenum.This additional
BleakBackendhas also the benefit for #1838 to simplify the conditional test execution depending on the detected backend.