-
Notifications
You must be signed in to change notification settings - Fork 23
Add AI instructions #1430
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
base: main
Are you sure you want to change the base?
Add AI instructions #1430
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # GitHub Copilot & Claude Code Instructions | ||
|
|
||
| This repository is `zwave-js-server-python`, a Python client library for [zwave-js-server](https://github.com/zwave-js/zwave-js-server/). It mirrors the structure and events of Z-Wave JS 1:1 (`Driver`, `Controller`, `Node`, etc.). | ||
|
|
||
| ## Scope | ||
|
|
||
| - One concern per PR. No bundling refactors, return-type changes, or renames into a feature/schema PR. Breaking changes go in their own PR. | ||
|
|
||
| ## Schema version | ||
|
|
||
| - Tests and fixtures reference `MAX_SERVER_SCHEMA_VERSION` / `MIN_SERVER_SCHEMA_VERSION`. Never hardcode the number. | ||
| - Helpers that send `schemaVersion` (e.g. `dump-state`) negotiate `min(MAX_SERVER_SCHEMA_VERSION, server_max_schema_version)`. No raw `MAX_SERVER_SCHEMA_VERSION`. | ||
|
|
||
| ## Typing | ||
|
|
||
| - Return annotation matches actual return value. If body returns `data["nvmId"]` (`int`), annotate `int`, not `dict`. | ||
| - No `Union` return when one type covers both known/unknown cases. Pick a type callers don't need to `isinstance`-discriminate. | ||
| - JSON keys are `str`. If model annotates `dict[int, ...]`, convert keys at construction (dict comprehension). Don't lie in the annotation. | ||
| - Public dict keyed by raw id (`int`), not model instance. Caller may not have the instance. | ||
|
|
||
| ## Dataclasses & caching | ||
|
|
||
| - No mutable module-level default on dataclass field. Copy in `__post_init__` (`list(DEFAULT_X)`). | ||
| - `@cached_property` returns immutable container (`tuple`, not `list`). | ||
| - New `@cached_property` requires `update()` to invalidate it, plus regression test: populate cache → `update()` → assert new value. | ||
| - No per-call `dict(entry)` in cached/parsing methods. Use `typing.cast` or narrow with `in` check. | ||
|
|
||
| ## Match existing patterns | ||
|
|
||
| Look for prior art in `zwave_js_server/model/` before inventing. Examples reviewers have flagged: | ||
|
|
||
| - `from_dict` classmethod parses server payload: `def from_dict(cls, data: <Type>DataType) -> Self`. Returns `Self`, not the explicit class. | ||
| - Rename unused parameters to `_param` (e.g. `event` in event handlers). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this. We don't have any parameters called
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The auto-generated instructions specifially mentioned
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this. We should keep the interface parameters always named the same. I suggest we remove this instruction and adjust the parameter names instead. |
||
|
|
||
| ## Docstrings | ||
|
|
||
| - No open design questions or upstream-review references. Move to `# TODO` in body. Docstring states current behaviour. | ||
|
|
||
| ## Tests | ||
|
|
||
| - All test function parameters typed. Concrete types (`Client`, `Driver`, `Controller`, `Node`), not `Any`. | ||
| - No conditions/branching in tests. Split or `pytest.mark.parametrize`. | ||
| - Duplicated test bodies → merge with `pytest.mark.parametrize`. | ||
| - Keyword arguments at call sites, unless the positional value is a variable with the same name as the parameter. | ||
| - Assert the full expected command payload, not just the return value. `mock_command` matches partially — incomplete assertion passes even when `nodeId`/`endpointIndex` drift. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| AGENTS.md |
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.
This is just best practice Python type annotation using modern Python.
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.
Right, but you flagged it during your most recent review. Opus 4.7 didn't follow best practices, so I think it needs a nudge in the right direction.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yes. Would it work to tell it to follow the latest type annotation best practices (proposed in PEPs) of the minimum supported version of the library?
typing.Selfwas added in Python 3.11. We support Python 3.12+.https://docs.python.org/3/library/typing.html#typing.Self
https://peps.python.org/pep-0673/
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.
@abmantis do you have any experience with this approach?
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.
Its hard to say. Since its already "old" (3.11) it may work, but it is probably going to be flaky/inconsistent since its very broad.
As a side note: don't we have mypy/pyright/ruff/... rules we can use for these?
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.
That would probably be the more reliable option.