backends: make timeout and disconnected_callback explicit kwargs#1994
backends: make timeout and disconnected_callback explicit kwargs#1994JPHutchins wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1994 +/- ##
===========================================
- Coverage 52.45% 52.45% -0.01%
===========================================
Files 43 43
Lines 4097 4099 +2
Branches 504 504
===========================================
+ Hits 2149 2150 +1
- Misses 1817 1818 +1
Partials 131 131
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors backend client constructors to pass timeout and disconnected_callback explicitly into BaseBleakClient, and loosens GitHub Actions PR trigger branch filtering.
Changes:
- Update
BaseBleakClient.__init__to take keyword-onlytimeoutanddisconnected_callbackparameters instead of pulling them from**kwargs. - Update multiple backend client constructors to accept/pass through the new parameters.
- Remove
pull_request.branchesrestrictions in CI workflows so PR workflows trigger for all target branches.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
bleak/backends/client.py |
Makes timeout and disconnected_callback explicit keyword-only args and updates callback contract docs. |
bleak/backends/winrt/client.py |
Updates WinRT backend constructor to accept/pass through timeout and disconnected_callback. |
bleak/backends/bluezdbus/client.py |
Updates BlueZ D-Bus backend constructor to accept/pass through timeout and disconnected_callback. |
bleak/backends/corebluetooth/client.py |
Updates CoreBluetooth backend constructor to accept/pass through timeout and disconnected_callback; adds Callable import. |
bleak/backends/p4android/client.py |
Updates Android backend constructor to accept/pass through timeout and disconnected_callback; adds Callable import. |
.github/workflows/format_and_lint.yml |
Removes PR branch filtering for format/lint workflow triggers. |
.github/workflows/build_and_test.yml |
Removes PR branch filtering for build/test workflow triggers. |
Comments suppressed due to low confidence (1)
bleak/backends/client.py:1
- Given the callback’s argument contract has changed, it would be good to add/adjust a unit test that exercises a disconnect path and asserts the callback invocation matches the documented signature. This helps catch regressions where a backend (or shared code) still calls the callback with the client argument.
# Created on 2018-04-23 by hbldh <henrik.blidh@nedomkull.com>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self, | ||
| address_or_ble_device: Union[BLEDevice, str], | ||
| services: Optional[set[str]] = None, | ||
| *, | ||
| timeout: float, | ||
| disconnected_callback: Callable[[], None] | None = None, | ||
| **kwargs: Any, | ||
| ): | ||
| super().__init__(address_or_ble_device, **kwargs) | ||
| super().__init__( | ||
| address_or_ble_device, | ||
| timeout=timeout, | ||
| disconnected_callback=disconnected_callback, | ||
| ) |
There was a problem hiding this comment.
Extra keyword arguments were already discarded before this PR: BaseBleakClient.__init__ itself takes **kwargs: Any and uses nothing beyond its named parameters, so forwarding them from the backend would not change the outcome. This PR's scope is just promoting timeout/disconnected_callback out of **kwargs; tightening the remaining catch-all is a separate cleanup.
| pull_request: | ||
| branches: [master, develop] | ||
| paths: ["bleak/**", "tests/**", ".github/workflows/build_and_test.yml", "pyproject.toml"] |
There was a problem hiding this comment.
Generally there's no reason to gate on open source, depending on volume of CI runs.
There was a problem hiding this comment.
This is an unrelated change so doesn't belong in this PR.
| pull_request: | ||
| branches: [master, develop] | ||
| paths: ["bleak/**", "tests/**", ".github/workflows/build_and_test.yml", "pyproject.toml"] |
There was a problem hiding this comment.
This is an unrelated change so doesn't belong in this PR.
| push: | ||
| branches: [master, develop] | ||
| pull_request: | ||
| branches: [master, develop] |
| address_or_ble_device: BLEDevice | str, | ||
| *, | ||
| timeout: float, | ||
| disconnected_callback: Callable[[], None] | None = None, |
There was a problem hiding this comment.
| disconnected_callback: Callable[[], None] | None = None, | |
| disconnected_callback: Callable[[], None] | None, |
Not much use in giving a default since we should always be passing it from the subclass's __init__ (in fact it could hide a bug if a new backend forgot to pass it along).
| timeout: float, | ||
| disconnected_callback: Optional[Callable[[], None]] = None, |
There was a problem hiding this comment.
Let's put these in the same order as bleak.BleakClient to keep things consistent.
There was a problem hiding this comment.
Yes, it is always timeout, disconnected_callback, unless I'm missing one someplace!
There was a problem hiding this comment.
Here these are after bluez when they should be before.
Replace the kwargs["timeout"] / kwargs.get("disconnected_callback") access
in BaseBleakClient with explicit keyword-only parameters and forward them
explicitly from each backend __init__ instead of threading **kwargs through.
disconnected_callback is required (no default) on BaseBleakClient so that a
backend which forgets to forward it fails loudly rather than silently
passing None. The backend constructors list the new parameters in the same
order as bleak.BleakClient and annotate disconnected_callback consistently.
No behavior change for the built-in backends or the public BleakClient,
which always pass timeout and disconnected_callback explicitly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JPHutchins
force-pushed
the
pairing-explicit-kwargs
branch
from
ce44cda to
4560596
Compare
| address_or_ble_device: Union[BLEDevice, str], | ||
| services: Optional[set[str]] = None, | ||
| *, | ||
| disconnected_callback: Callable[[], None] | None = None, |
There was a problem hiding this comment.
| disconnected_callback: Callable[[], None] | None = None, | |
| disconnected_callback: Callable[[], None] | None, |
| address_or_ble_device: Union[BLEDevice, str], | ||
| services: Optional[set[str]] = None, | ||
| *, | ||
| disconnected_callback: Callable[[], None] | None = None, |
There was a problem hiding this comment.
| disconnected_callback: Callable[[], None] | None = None, | |
| disconnected_callback: Callable[[], None] | None, |
| services: Optional[set[uuid.UUID]], | ||
| **kwargs, | ||
| *, | ||
| disconnected_callback: Callable[[], None] | None = None, |
There was a problem hiding this comment.
| disconnected_callback: Callable[[], None] | None = None, | |
| disconnected_callback: Callable[[], None] | None, |
| disconnected_callback: Callable[[], None] | None = None, | ||
| timeout: float, |
There was a problem hiding this comment.
| disconnected_callback: Callable[[], None] | None = None, | |
| timeout: float, | |
| timeout: float, | |
| disconnected_callback: Callable[[], None] | None, |
Split out of #1990.
What
Makes the previously-implicit
timeout/disconnected_callbackcontract explicit on the backend clients instead of threading them through**kwargs:BaseBleakClient.__init__takestimeoutanddisconnected_callbackas explicit keyword-only parameters.disconnected_callbackdocstring: the backend-level callable takes no arguments (signature isCallable[[], None]), matching how backends invoke it — the previous "must take one argument (the client)" text described the public facade, not this interface.This makes the constructor signatures self-documenting and is the groundwork for adding
pairing_callbacksas a sibling explicit parameter in a follow-up.Note for review
This also drops the
branches: [master, develop]filter from thepull_request:trigger inbuild_and_test.ymlandformat_and_lint.ymlso CI runs on every PR (it was needed to run CI on the stacked split PRs). If you'd rather keep that filter on upstream, I'm happy to pull those two lines back out of this PR.Testing
🤖 Generated with Claude Code