Add mypy strict + tighten get_job() to JobInfo | None + py.typed marker#167
Conversation
Adds mypy in strict mode as a CI step. The library previously had correct TypedDict declarations on its public methods but the implementations all returned `Any` (the result of `response.json()`), so mypy was unable to verify consumer code at the boundary. With `Any → TypedDict` mypy is lenient and silently accepts the conversion; with `Any → list[Storage]` (added in 2.2.0) it does not — that is the first time the leak surfaced for a downstream consumer, even though every GET method had the same flaw. Architectural choice: `cast()` at each return site rather than runtime validation (pydantic / msgspec / hand-rolled TypeGuards). Reasoning: - pyprusalink is a thin API wrapper, not a data-modelling library. Pulling in a validation library (~50KB+ C extension) is overkill for a wrapper that calls a stable, well-defined Prusa API. - `cast()` is the dominant pattern in the Home Assistant ecosystem for this exact situation (TypedDict from JSON) and matches what HA core does internally. - Trade-off accepted: `cast()` does not validate at runtime; if the Prusa API ever changes shape, errors surface as `KeyError` / `AttributeError` in consumer code rather than at the library boundary. For a stable upstream API this is acceptable; if it becomes a problem, runtime validation can be added later without changing the public types. Strict mode is enabled so future contributions cannot quietly weaken this — any new method that leaks `Any` will fail CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces strict static type checking to the project’s CI via mypy --strict and addresses the resulting type-soundness gaps by tightening API response typing (primarily around response.json() returning Any).
Changes:
- Add
mypy --strictconfiguration and run it in GitHub Actions CI. - Use
typing.cast(...)atresponse.json()call sites to preventAnyfrom leaking into declaredTypedDictreturn types. - Refine a few overly-broad
dictannotations todict[str, Any]for better strict-mode compatibility.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds mypy to lint extras and enables strict mypy configuration. |
.github/workflows/test.yml |
Runs mypy as part of CI alongside existing linters/tests. |
pyprusalink/__init__.py |
Adds cast(...) to typed API wrapper return paths to avoid Any leakage. |
pyprusalink/types.py |
Tightens JobFilePrint.meta typing to `dict[str, Any] |
pyprusalink/client.py |
Tightens request JSON payload typing to `dict[str, Any] |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change get_job() return type to JobInfo | None and return None on 204 instead of casting an empty dict to JobInfo. Callers can now check for None explicitly instead of guessing whether a TypedDict has its required keys. - Add PEP 561 py.typed marker so downstream type checkers actually see the TypedDicts. Set zip-safe = false (per mypy/setuptools guidance for typed packages) and include py.typed via setuptools package-data. - Drop the misspelled 204 comment; the new docstring makes the no-job case explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
agners
left a comment
There was a problem hiding this comment.
So with returning None now we essentially change the APU. According to semantic versioning this would mean the next release needs to be a major version bump. I am fine with it, just saying... If we have other breaking API changes in mind, let's introduce them now too.
Per @agners on home-assistant-libs#167: `zip-safe` is a setuptools-egg-era flag and modern wheel-based installs ignore it. The original change was based on mypy/setuptools docs that still mention `zip-safe = false` for typed packages, but in practice py.typed is reliably accessible from extracted wheels regardless of this flag. Cleaner to drop it entirely. Wheel build verified to still include `pyprusalink/py.typed`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Thanks. Agree we should do a major-bump. I did a thorough check if there is anything else that could be in a bump. Already in this PR (the actual driver for 3.0.0):
Worth considering: PrinterInfo: migrate T | None → NotRequired[T] to match VersionInfo. The API actually omits absent fields rather than returning None, so the current typing is dishonest. HA-side already accesses PrinterInfo defensively via .get() in most places — the migration makes the contract match reality without forcing real changes downstream. Twelve fields, but straightforward. Potential, want your take: jobId → job_id rename on the four job-control methods, for PEP 8 and consistency with cancel_transfer(transfer_id). HA-core calls them positionally so HA doesn't benefit, but the inconsistency is a wart. Cheap to fold in if you'd like it cleaned up; otherwise happy to leave it. Otherwise no other breaking changes on my roadmap right now. #155 should warrant a new major version in the future, but not ready for that. Lean: ship 3.0.0 with what's in this PR + the PrinterInfo migration, and add the jobId rename if you want it. |
Yeah let's do PrinterInfo and jobId rename as well. |
The /api/v1/info endpoint actually omits absent fields rather than returning None — the original `T | None` typing was a lie that misled consumers into expecting `info["x"]` to always work. NotRequired makes the contract honest: fields may be absent, and consumers must use .get() or membership checks. Older Buddy firmware versions and edge configurations (e.g. printers not in farm mode) omit several fields. Documented in the docstring. Verified all 12 field types against Prusa-Link-Web's authoritative OpenAPI spec (no `required` list, so all properties are optional). Style matches the existing `NotRequired[T]` on `VersionInfo`. Breaking change for strict-typed consumers that index missing fields; they need to switch to `.get()`. Targeted at the 3.0.0 release (see #167). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings the four job-control methods (cancel_job, pause_job, resume_job, continue_job) in line with PEP 8 and with cancel_transfer(transfer_id) which already uses snake_case. Breaking change for callers using keyword arguments (cancel_job(jobId=42)). Home Assistant calls these methods positionally via `lambda api: api.cancel_job` in the button entity descriptions, so no HA-side changes are needed. Targeted at the 3.0.0 release (see #167). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Rewrite README for the 3.0 release The old three-line README didn't say much beyond the package name. The new one covers what someone landing on the PyPI page actually needs: - positioning: thin async wrapper, primary consumer is HA - requirements and async-only / httpx caveat - quickstart with credential note - public API table by endpoint - exception hierarchy and example - type contract: NotRequired vs T | None, why we use cast() instead of pydantic/msgspec - semver policy (TypedDict shape changes = breaking) - development setup and the opt-in integration test invocation Some of what's described — mypy strict, the cast() pattern, get_job() returning None, get_transfer() returning None — is landing as part of 3.0.0 in #167/#169/#170, so this README is sized to match that state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Reframe the README intro per agners' review - Drop the "intentionally a thin wrapper" framing. As agners pointed out, that codifies a present-state property rather than a design principle — the library shape should be whatever serves the HA integration best, and may evolve. - Replace with "API shape decisions are weighted toward serving the HA integration", and qualify the no-validation/no-retry note as a current state ("Today... but the shape may evolve"). - Fix "PrusaLink v2 API" — the API isn't versioned that way; replace with "PrusaLink HTTP API" and explicitly note the legacy paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Add Copilot code review instructions Captures the conventions Copilot's PR reviewer should follow when reviewing this repo: review-style rules at the top (avoid commenting on lint/formatting since CI already enforces it), public API conventions (TypedDicts + cast at JSON boundaries is intentional, NotRequired over T | None, T | None reserved for "no data" return paths), test layout (respx + optional integration marker), and semver expectations for TypedDict shape changes. Standalone — no generator script like home-assistant/core has; the file is small enough to maintain by hand. Some of the conventions described (mypy strict, cast() pattern, PrinterInfo NotRequired migration, get_job() -> JobInfo | None) are landing as part of the upcoming 3.0.0 release in #167 / #169 / #170. The file describes the end state; once 3.0.0 ships, everything in here will be true on main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review on Copilot instructions - Drop "PrusaLink v2 API" framing — agners pointed out that the endpoints aren't versioned that way and the library also covers a few legacy paths (/api/version, /api/printer). Replace with "PrusaLink HTTP API" and explicitly mention both endpoint families. - Drop the blanket "suggest fixes at the library level" rule. As agners noted, that's case-by-case rather than a general principle. - Drop "thin wrapper" framing in favour of "the shape is weighted toward what serves the HA integration best". - Add concrete examples of helpful vs unhelpful Copilot feedback so the rule list isn't just abstract dos/don'ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds
mypy --strictas a CI step, fixes the type-soundness issues it surfaces, and addresses two correctness gaps caught in review.The library declared correct TypedDict return types on its public methods, but the implementations all returned
Any(the result ofresponse.json()). Mypy is lenient withAny → TypedDictso the leak was invisible, butAny → list[Storage](added in 2.2.0) is strict — that's how the issue surfaced when wiring up the newget_storage()method downstream in Home Assistant core. Every other GET method had the same latent flaw.Why now
We discovered this while writing the storage-sensors integration in HA. Without fixing it at the source, every downstream consumer would have to
cast()results back into the declared types — type debt distributed across consumers. Cleaner to fix once at the boundary.get_job()signature change (review feedback)The original code did
cast(JobInfo, {})on the 204 branch, which lies to the type checker — an empty dict does not satisfyJobInfo's required keys. Changed toJobInfo | None, returningNoneon 204. This matches the existingget_transfer() -> Transfer | Nonepattern.This is a minor breaking change at the type level. Runtime impact for sane consumers is nil — anyone reading
job["progress"]on the empty-dict return was already crashing today; the new contract just makes the no-job state explicit. HA-side will need a one-linedata is not Noneguard in its base entity'savailablecheck; that change rides along with the eventual 2.2.1 bump there.py.typedmarker (review feedback)Adding
pyprusalink/py.typed(PEP 561) so downstream type checkers actually see the TypedDicts. Without it, the strict-typing work in this PR is invisible to consumers — installed distributions would be treated as untyped. Includes:pyprusalink/py.typed(full typing marker)[tool.setuptools.package-data] pyprusalink = ["py.typed"]to ensure inclusion in the wheelzip-safe = falseper setuptools/mypy guidance for typed packagesWhy
cast()(and not pydantic / msgspec / TypeGuards)cast()at each return site is the pragmatic choice for a thin API wrapper:cast()# type: ignoreTrade-off accepted:
cast()does not validate at runtime. If the Prusa API ever changes shape, errors surface asKeyError/AttributeErrorin consumer code rather than at the library boundary. For a stable upstream API this is acceptable; runtime validation can be layered on later without changing the public types.Why strict from the start
--strictrather than gradual tightening: with only 10 errors on a small codebase, fixing them in one pass is cheaper than introducing a slow ratchet. Strict mode then prevents future contributions from quietly weakening typing — any new method that leaksAnywill fail CI.Changes
pyproject.toml:mypy==2.0.0inlintextras;[tool.mypy] strict = true;package-dataforpy.typed;zip-safe = false.github/workflows/test.yml: newmypysteppyprusalink/py.typed: PEP 561 marker (new, empty)pyprusalink/__init__.py:cast(...)at eachresponse.json()return;get_job() -> JobInfo | NonereturningNoneon 204pyprusalink/types.py:dict | None→dict[str, Any] | NoneonJobFilePrint.metapyprusalink/client.py: samedict[str, Any]fix onrequest(..., json_data=...)tests/: updatedget_jobno-job assertion from== {}tois NoneSuggested release
Suggesting 2.2.1 as a patch release once this lands. Runtime is fully backward compatible for sane consumers; the typing contract becomes more honest. HA core will bump to 2.2.1 in a follow-up PR that also adds the small
data is not Noneguard.Test plan
mypy— Success: no issues found in 4 source filespytest tests/— 22 passedpyprusalink/py.typed🤖 Generated with Claude Code