Skip to content

fix: make Signal.fire keyword-only to type the dispatch contract #1784

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.

Sign up for GitHub

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

Open
bluetoothbot wants to merge 9 commits into
base: master
Choose a base branch
from
Open

fix: make Signal.fire keyword-only to type the dispatch contract #1784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
769d9e5
fix: type Signal dispatch contract with Protocol
bluetoothbot May 26, 2026
77c8c08
fix: make Signal.fire keyword-only to type the dispatch contract
bluetoothbot Aug 26, 2026
6d21163
test: silence mypy arg-type on positional Signal.fire call
bluetoothbot Aug 26, 2026
255510b
fix: resolve CI failures on #1784 (attempt 1)
bluetoothbot Aug 26, 2026
7193b54
chore: drop out-of-scope cython version cap from build requires
bluetoothbot Aug 26, 2026
4f16168
fix: resolve CI failures on #1784 (attempt 1)
bluetoothbot Aug 26, 2026
aed6b41
chore: drop out-of-scope cython cap and ci release-dry-run change
bluetoothbot Aug 28, 2026
b127121
test: pin unknown-kwarg rejection in Signal.fire test
bluetoothbot Aug 28, 2026
c789ca9
fix: accept and discard extra kwargs in Signal.fire for back compat
bluetoothbot Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/zeroconf/_services/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,22 @@ class Signal:
def __init__(self) -> None:
self._handlers: list[Callable[..., None]] = []

def fire(self, **kwargs: Any) -> None:
def fire(
self,
*,
zeroconf: Zeroconf,
service_type: str,
name: str,
state_change: ServiceStateChange,

@bdraco bdraco Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need a **kwargs: Any throw away as well for back compat?

@bluetoothbot bluetoothbot Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Important

Depends on how much compat you want.

**kwargs: Any throwaway costs little and keeps most of this PR's value: typo protection survives, because nmae= gets absorbed by kwargs while name stays required, so mypy still flags a missing argument. Only loss: extra keys pass through to handlers again — same as pre-change behaviour.

Full back-compat needs defaults too. A caller firing only its own keys still breaks on four required params, and defaults would kill the missing-argument check entirely.

Only in-tree caller: browser.py:743. Adopting **kwargs means rewriting test_signal_fire_rejects_unknown_kwarg (tests/test_services.py:306) to assert forwarding, not rejection.

Which way do you want it?

@bdraco bdraco Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably just accept **kwargs and throw them away in case someone passes garbage it doesn't break

**kwargs: Any,
) -> None:
for h in self._handlers[:]:
h(**kwargs)
h(
zeroconf=zeroconf,
service_type=service_type,
name=name,
state_change=state_change,
)

@property
def registration_interface(self) -> SignalRegistrationInterface:
Expand Down
78 changes: 78 additions & 0 deletions tests/test_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,81 @@ def dummy():

with pytest.raises(ValueError):
interface.unregister_handler(dummy)


def test_signal_fire_dispatches_documented_kwargs():
"""Signal.fire forwards the four documented kwargs to handlers."""
signal = r.Signal()
captured: list[dict[str, Any]] = []

def handler(
*,
zeroconf: Any,
service_type: str,
name: str,
state_change: r.ServiceStateChange,
) -> None:
captured.append(
{
"zeroconf": zeroconf,
"service_type": service_type,
"name": name,
"state_change": state_change,
}
)

signal.registration_interface.register_handler(handler)
sentinel = object()
signal.fire(
zeroconf=sentinel, # type: ignore[arg-type]
service_type="_http._tcp.local.",
name="x._http._tcp.local.",
state_change=r.ServiceStateChange.Added,
)

assert captured == [
{
"zeroconf": sentinel,
"service_type": "_http._tcp.local.",
"name": "x._http._tcp.local.",
"state_change": r.ServiceStateChange.Added,
}
]


def test_signal_fire_discards_unknown_kwarg():
"""Signal.fire accepts extra keyword args and does not forward them."""
signal = r.Signal()
captured: list[dict[str, Any]] = []
signal.registration_interface.register_handler(lambda **kw: captured.append(kw))

signal.fire(
zeroconf=None, # type: ignore[arg-type]
service_type="_http._tcp.local.",
name="x._http._tcp.local.",
state_change=r.ServiceStateChange.Added,
bogus=1,
)

assert captured == [
{
"zeroconf": None,
"service_type": "_http._tcp.local.",
"name": "x._http._tcp.local.",
"state_change": r.ServiceStateChange.Added,
}
]


def test_signal_fire_rejects_positional_args():
"""Signal.fire is keyword-only, so positional args are rejected."""
signal = r.Signal()
signal.registration_interface.register_handler(lambda **_: None)

with pytest.raises(TypeError):
signal.fire( # type: ignore[misc]
None, # type: ignore[arg-type]
"_http._tcp.local.",
"x._http._tcp.local.",
r.ServiceStateChange.Added,
)