GeezerVison

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (77f8cf0) to head (c789ca9).
⚠️ Report is 3 commits behind head on master.

@@           Coverage Diff           @@
##           master    #1784   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          33       33           
  Lines        3839     3839           
  Branches      553      553           
=======================================
  Hits         3832     3832           
  Misses          5        5           
  Partials        2        2           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Merging this PR will not alter performance

✅ 24 untouched benchmarks

_{Comparing bluetoothbot:koan/fix-issue-1779 (c789ca9) with master (77f8cf0)¹}

Coverage is incomplete. Ci is failing

@bluetoothbot Rr

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Stats

3 files changed, 108 insertions(+), 8 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

@bdraco: Coverage is incomplete. Ci is failing

Confirmed, both still open.

Coverage: Protocol __call__ stub never executes. Body ... at _services/__init__.py:61 uncoverable by design. Fix via # pragma: no cover, or add \.\.\. to exclude_lines in pyproject.toml.

CI: test plan skipped mypy and pre-commit. Suspect # type: ignore[misc] at tests/test_services.py:326 — positional call also emits [call-arg], unsuppressed.

Cleanest fix drops Protocol entirely. Keep _handlers: list[Callable[..., None]], type only fire params. Same checking at browser.py:740. No cast, no export, no coverage gap.

Note: last force-push rebased only. Feedback step aborted on session limit.

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• Net diff now matches reviewer's proposed variant exactly. src/zeroconf/__init__.py back to master state.
• Dropped ServiceStateChangeHandler Protocol from zeroconf._services, per @bdraco — the keyword-only fire() signature alone delivers the entire static-checking gain (the browser.py:740 call site), since register_handler accepted Callable[..., None] and cast anyway. Removes the uncoverable Protocol stub, the cast()s, and a Protocol class inside a Cythonized module.
• Reverted _handlers / SignalRegistrationInterface.__init__ to list[Callable[..., None]]; removed both cast() calls in register_handler / unregister_handler.
• Removed the ServiceStateChangeHandler import and __all__ entry from src/zeroconf/__init__.py, so no new symbol is committed to the public surface or to docs/api.rst's automodule :members:.
• Deleted test_service_state_change_handler_protocol_exported — the hasattr assertion tested a symbol that no longer exists.
• Kept the two TypeError tests (rejects_unknown_kwarg, rejects_positional_args) — the reviewer explicitly judged they earn their keep by pinning the generated signature under the compiled build.
• Not run: tests (caller handles). Ruff lint + format clean on all three files.
• CI fix: Applied pre-push CI fix
• CI fix: Pre-push CI fix applied

Stats

2 files changed, 83 insertions(+), 3 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

@bdraco: @bluetoothbot Rr

Re-reviewed at head 1220891.

Protocol gone. Diff now matches your requested variant: keyword-only fire(), _handlers back to Callable[..., None], src/zeroconf/__init__.py untouched. Uncoverable Protocol stub gone, so patch coverage gap gone.

mypy ignores at tests/test_services.py:326-327 now cover both emitted codes (misc on call, arg-type on None). Could not run mypy here — please confirm via pre-commit run --all-files.

Only remaining nit: retitle. Suggest fix: make Signal.fire keyword-only to type the dispatch contract. Current title names a Protocol no longer present, and squash-merge sends it to changelog.

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• PR metadata updated. No code changes needed.
• Retitled PR fix: make Signal.fire keyword-only to type the dispatch contract #1784 to fix: make Signal.fire keyword-only to type the dispatch contract, per suggestion Create API with underscores instead of camel case, deprecate camel case API #2 — old title described a Protocol that no longer exists in the diff and would land verbatim on master via squash-merge.
• Rewrote PR body to match the actual diff: keyword-only fire, explicit kwarg forwarding, three tests. Removed the stale ServiceStateChangeHandler Protocol, cast(), and src/zeroconf/__init__.py claims.
• Added a "Behaviour notes" section to the body covering both tightenings raised in suggestion Fix ServiceInfo __repr__ #1: Signal reachable from the public package so third-party fire() calls with other kwargs now raise TypeError, and Cython annotation_typing making service_type/name runtime-typed in the compiled wheel while pure Python forwards non-str silently.

Not changed (and why)

• Suggestion Fix ServiceInfo __repr__ #1 requested no code change — explicitly "flagging so the tightening is a decision rather than a side effect". Verified both claims against the current tree (build_ext.py TO_CYTHONIZE includes _services/__init__.py, __init__.pxd declares cdef class Signal, src/zeroconf/__init__.py:57 imports Signal). Decision: keep the tightening; documented in the PR body as the reviewer asked.

Stats

2 files changed, 83 insertions(+), 3 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• Cap reverted. PR now touches only Signal.fire + tests.
• Reverted the Cython>=3.0.8,<3.3 cap and its comment in [build-system].requires (pyproject.toml), per warning ＃1 — out of scope for this PR, unmentioned in the description, and a no-op for PR CI since poetry.lock pins cython 3.2.9 and no PR-triggered job resolves build-system.requires. PR is back to Signal.fire + tests only.
• CI fix: Applied pre-push CI fix
• CI fix: Pre-push CI fix applied

Not changed (and why)

• Suggestion ＃2 (also cap dev-group cython = ">=3.2.9,<3.3"): moot now that the cap is gone. Capping both belongs in the separate PR the reviewer proposed, together with the upstream Cython issue link. The .py-subscripted-generic vs .pxd cython.set incompatibility spans ~15 sites, so it needs its own scoped change — not this one.

Stats

3 files changed, 94 insertions(+), 6 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• Diff now scoped to Signal.fire + tests only.
• Dropped the Cython>=3.0.8,<3.3 cap and its comment from pyproject.toml, restoring master's Cython>=3.0.8. Verified df947b2 ("fix: build with cython 3.3.0", fix: build with cython 3.3.0 #1820) is an ancestor of this branch's base f5056e6 (upstream/master head), so the cap would have re-imposed a bound master already removed.
• Reverted .github/workflows/ci.yml to master: release-dry-run uses the SHA-pinned python-semantic-release container action again, no pip install and no gitpython<3.1.60 pin. Removes the dry-run/release asymmetry the reviewer flagged and the unmentioned release-infra change from a fix:-titled PR.
• Diff vs upstream/master is now only src/zeroconf/_services/__init__.py and tests/test_services.py, matching the PR description.
• Note for the human: the GitPython 3.1.60 / Actor.name_email_regex failure, if real, still affects release (ci.yml:218) and publish-action (ci.yml:228) — both unpinned. That needs its own ci: PR pinning all three jobs together, not just the dry run. I could not verify the GitPython API removal (no network from this shell).

Stats

2 files changed, 83 insertions(+), 3 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• Done.
• Reworked test_signal_fire_rejects_unknown_kwarg per suggestion ＃1: pass all four valid kwargs plus a bogus fifth, and pin the error with match="unexpected keyword argument", so the test distinguishes out-of-contract keyword rejection from arity enforcement.

Not changed (and why)

• Suggestion ＃2 (handler side still Callable[..., None]): reviewer explicitly said "No change required". Typing the handler list needs a ServiceStateChangeHandler Protocol in a Cythonized cdef class module — a new public name and awkward under Cython, out of scope here. PR description wording is a body edit, not a code change; flagging for the human to trim "or a missing parameter at a handler" or file a follow-up.
• Earlier review items on the Cython version cap and ci.yml release-dry-run: already reverted in 6efab64 on this branch.

Stats

2 files changed, 84 insertions(+), 3 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

PR Review — fix: make Signal.fire keyword-only to type the dispatch contract

Clean, well-scoped change — blocked only on @bdraco's open back-compat question about a throwaway **kwargs.

Strengths: the keyword-only signature matches the sole internal caller (browser.py:743-748) argument-for-argument, so runtime dispatch is byte-for-byte unchanged; the TYPE_CHECKING-only Zeroconf import plus from __future__ import annotations mirrors the pattern already compiling fine in the same Cythonized package (browser.py:263, 348, 570); no .pxd edit is needed because fire is a plain def and _services/__init__.pxd declares only cdef list _handlers; git diff master...HEAD is exactly the two files the description names. The prior round's pyproject.toml Cython cap and ci.yml release-dry-run regressions stay reverted, and suggestion ＃1 from last round is properly applied — test_signal_fire_rejects_unknown_kwarg now passes all four valid kwargs plus a bogus fifth with match="unexpected keyword argument", which also matches Cython's compiled error text, so it pins the right condition in both builds.

• 🟡 Signal is importable from the top-level package, so the fixed signature is a back-compat break for out-of-tree callers. @bdraco requested changes on this exact line; a **kwargs: Any throwaway would keep the typo protection (a misspelled name= still fails mypy as a missing required arg) at the cost of the new unknown-kwarg test — needs a maintainer decision, not a code fix I can pick for you.
• 🟢 Carried over and already deferred: handler registration remains Callable[..., None], so the description's "missing parameter at a handler" half is still unchecked; trim the wording or file a follow-up.
• Unverified: no mypy in this shell, so the placement of # type: ignore[misc] / [call-arg] in the new tests could not be confirmed. warn_unused_ignores = false (pyproject.toml:255) makes a surplus ignore harmless; a misplaced one would surface in the pre-commit mypy hook.

✅ Resolved since last review (1)

🟡 Important

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

Checklist

• PR description matches the diff (no unmentioned changes or scope creep)
• Runtime dispatch behaviour unchanged for the internal call site
• Cython .pxd updated where required
• New behaviour covered by tests (dispatch, unknown kwarg, positional)
• Backward compatibility of the public/importable surface resolved — warning ＃1
• No security-sensitive surface touched

Automated review by Kōan (Claude) HEAD=0ba635c 3 min 39s

Rebase with requested adjustments

Branch koan/fix-issue-1779 was rebased onto master and review feedback was applied.

Changes applied

• Lint clean. .pxd needs no change — fire is a plain def method, not declared there.
• Added throwaway **kwargs: Any to Signal.fire per @bdraco, so out-of-tree callers passing extra keys no longer get TypeError. Extras are absorbed and discarded, not forwarded — matches "throw them away in case someone passes garbage it doesn't break"; forwarding garbage would still blow up in handlers.
• Restored the Any import in src/zeroconf/_services/__init__.py.
• Replaced test_signal_fire_rejects_unknown_kwarg with test_signal_fire_discards_unknown_kwarg, asserting handlers receive only the four documented kwargs when an extra is passed.
• Static win retained: the four params stay required keyword-only, so a typo (nmae=) is still a mypy "missing argument". test_signal_fire_rejects_positional_args unchanged and still valid.

Stats

2 files changed, 93 insertions(+), 2 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan