-
Notifications
You must be signed in to change notification settings - Fork 8
attributes: @attr decorator sugar over the getter/setter constructors
#423
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9a907d9
attributes: @attr decorator sugar over the getter/setter constructors
claude 8064b70
chore: amend helper function to not do the job of the caller in raisi…
shihab-dls cdcb35f
tests: fix code smells in tests such as useless lines and more than o…
shihab-dls a5b7bf7
chore: add claude.md file with issues found in this branch
shihab-dls File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| # CLAUDE.md | ||
|
|
||
| Conventions for this repository. Follow these when writing or reviewing code and tests. | ||
|
|
||
| ## Helper functions and exceptions | ||
|
|
||
| - A helper function must **not** be passed a parameter (e.g. `kind`) whose only purpose | ||
| is to be interpolated into the message of an exception it raises. That parameter | ||
| exists purely to serve one or more call-sites' error-reporting needs, which means the | ||
| helper is doing the call-site's job for it. | ||
| - If different call-sites need different exceptions raised (different types and/or | ||
| different messages), do not thread a parameter into the helper to cover every case. | ||
| Instead, let the helper raise its own plain/generic exception with no caller-supplied | ||
| wording, and have each call-site `catch` it and `raise ... from ...` with the | ||
| type/message it actually needs. | ||
|
|
||
| ```python | ||
| # Bad: helper takes `kind` purely to phrase its own exception message | ||
| def _check_positive(value: float, kind: str) -> None: | ||
| if value <= 0: | ||
| raise ValueError(f"{kind} must be positive, got {value}") | ||
|
|
||
| _check_positive(period, kind="period") | ||
|
|
||
| # Good: helper raises a plain exception; call-site adds whatever context it needs | ||
| def _check_positive(value: float) -> None: | ||
| if value <= 0: | ||
| raise ValueError(f"must be positive, got {value}") | ||
|
|
||
| try: | ||
| _check_positive(period) | ||
| except ValueError as e: | ||
| raise ConfigError(f"invalid period in trigger config: {e}") from e | ||
| ``` | ||
|
|
||
| - A helper function must **not** be passed a parameter like `expected` or `skip` that | ||
| tells it about the *arity or shape of the call-site* (e.g. "how many items did you | ||
| expect", "should this check be skipped"). Parameters like these are a sign that the | ||
| check itself belongs in the caller, not the helper. Move the check up: | ||
|
|
||
| ```python | ||
| # Bad: helper is making a decision that belongs to the caller | ||
| def _check_length(items, expected=None): | ||
| if expected is not None and len(items) != expected: | ||
| raise ValueError(...) | ||
|
|
||
| # Good: caller owns the decision, helper just does the one thing it's for | ||
| if len(items) != expected: | ||
| raise ValueError(...) | ||
| _check_length(items) | ||
| ``` | ||
|
|
||
| Rule of thumb: a helper's parameters should describe *what it's being asked to | ||
| validate/produce*, never *whether/how the caller wants it validated*. | ||
|
|
||
| ## Tests: `pytest.raises` | ||
|
|
||
| - The `with pytest.raises(...):` block should contain the **minimal code that raises | ||
| the exception** — ideally a single line, and ideally just the call under test. | ||
| - Any setup needed to *put the system in a state* where that call will raise must | ||
| happen **outside** and **before** the `pytest.raises` block, not inside it. | ||
|
|
||
| ```python | ||
| # Bad: setup is inside the raises block | ||
| with pytest.raises(ValueError): | ||
| controller = Device() | ||
| controller.configure(bad_value) | ||
|
|
||
| # Good: setup happens first, only the failing call is inside the block | ||
| controller = Device() | ||
| with pytest.raises(ValueError): | ||
| controller.configure(bad_value) | ||
| ``` | ||
|
|
||
| This keeps the assertion precise: if setup itself started raising unexpectedly, the | ||
| test should fail with an ordinary traceback, not be masked as a (possibly | ||
| coincidental) pass inside `pytest.raises`. | ||
|
|
||
| ## Tests: no irrelevant lines | ||
|
|
||
| - Every line in a test should be there because it affects the test's outcome, given | ||
| what the test's name says it's checking. If removing a line wouldn't change whether | ||
| the test passes or fails, it doesn't belong. | ||
| - Before adding or keeping a line in a test, check it against the test name: does this | ||
| line change the behavior being verified? If not, delete it. | ||
|
|
||
| ```python | ||
| # Bad: post_initialise() doesn't affect this test's assertion | ||
| def test_matching_type_hint_is_satisfied_by_the_decorated_attribute(): | ||
| class Device(Controller): | ||
| label: AttrR[str] # pyright: ignore[reportRedeclaration] | ||
|
|
||
| @attr | ||
| async def label(self) -> str: | ||
| return "x" | ||
|
|
||
| controller = Device() | ||
| controller.post_initialise() # irrelevant — remove | ||
|
|
||
| assert isinstance(controller.label, AttrR) | ||
|
|
||
| # Good | ||
| def test_matching_type_hint_is_satisfied_by_the_decorated_attribute(): | ||
| class Device(Controller): | ||
| label: AttrR[str] # pyright: ignore[reportRedeclaration] | ||
|
|
||
| @attr | ||
| async def label(self) -> str: | ||
| return "x" | ||
|
|
||
| controller = Device() | ||
|
|
||
| assert isinstance(controller.label, AttrR) | ||
| ``` | ||
|
|
||
| This keeps tests readable as documentation: every line is evidence for the claim in | ||
| the test's name, not incidental noise carried over from copy-pasting another test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| # FastCS for PyTango Users | ||
|
|
||
| If you write Tango Device Servers with PyTango, the shape of a FastCS controller | ||
| will already be familiar: a class, some attributes, some commands. This page | ||
| pairs the PyTango spelling with the FastCS one, so you can carry what you know | ||
| across. | ||
|
|
||
| The headline difference is that a FastCS controller is not tied to Tango. The | ||
| same class is served over Tango, EPICS (Channel Access or PV Access), REST and | ||
| GraphQL - see [](./multiple-transports.md). | ||
|
|
||
| ## Hello world | ||
|
|
||
| PyTango's simplest attribute is one decorated getter: | ||
|
|
||
| ```python | ||
| from tango.server import Device, attribute | ||
|
|
||
|
|
||
| class PowerSupply(Device): | ||
| @attribute | ||
| def voltage(self) -> float: | ||
| return 2.5 | ||
| ``` | ||
|
|
||
| FastCS says the same thing with `@attr`: | ||
|
|
||
| ```python | ||
| from fastcs.attributes import attr | ||
| from fastcs.controllers import Controller | ||
|
|
||
|
|
||
| class PowerSupply(Controller): | ||
| @attr | ||
| async def voltage(self) -> float: | ||
| return 2.5 | ||
| ``` | ||
|
|
||
| Two differences to notice: | ||
|
|
||
| - The getter is `async`. FastCS controllers run on one event loop, so a getter | ||
| that talks to a device awaits it rather than blocking every other attribute. | ||
| - The datatype comes from the return annotation. There is no `dtype=` keyword to | ||
| keep in step with the code - `-> float` is one real annotation, checked by your | ||
| type checker as well as by FastCS. | ||
|
|
||
| ## Writing as well as reading | ||
|
|
||
| PyTango pairs a getter with a `@x.write` method (or `@x.setter` in the | ||
| `attribute` decorator form). FastCS mirrors `@property`: | ||
|
|
||
| ```python | ||
| class PowerSupply(Controller): | ||
| @attr(units="V", precision=3) | ||
| async def voltage(self) -> float: | ||
| """Output voltage.""" | ||
| return float(await self._conn.query("V?")) | ||
|
|
||
| @voltage.setter | ||
| async def voltage(self, value: float) -> None: | ||
| await self._conn.send(f"V={value}") | ||
| ``` | ||
|
|
||
| A getter alone gives you a read-only `AttrR`; adding a setter makes the same | ||
| name an `AttrRW`. There is no write-only decorator - a write-only attribute is | ||
| rare enough to be written longhand as `AttrW(setter=...)`. | ||
|
|
||
| The getter's docstring becomes the attribute's description, and keyword | ||
| arguments to `@attr` are the attribute's metadata - `units`, `precision`, | ||
| `limits`, `group`, `description`. They are checked against the datatype the | ||
| getter returns, so `precision` on a `-> str` getter is an error rather than a | ||
| field that is silently ignored. | ||
|
|
||
| :::{note} | ||
| Type checkers special-case the builtin `property` but not decorators that | ||
| imitate it, so pyright reports the getter as *obscured by a declaration of the | ||
| same name*, and mypy as *already defined*. The two declarations are deliberate, | ||
| so silence it at the getter: | ||
|
|
||
| ```python | ||
| @attr(units="V") | ||
| async def voltage(self) -> float: # pyright: ignore[reportRedeclaration] | ||
| ... | ||
| ``` | ||
|
|
||
| Only read-write attributes need this. A read-only `@attr` declares its name | ||
| once and needs nothing. | ||
| ::: | ||
|
|
||
| ## Deciding when a value is read | ||
|
|
||
| PyTango polls an attribute on a period configured per device, outside the code. | ||
| In FastCS the schedule is part of the declaration, and is the same | ||
| `Polled`/`NotPolled` vocabulary the procedural form uses: | ||
|
|
||
| ```python | ||
| from fastcs.attributes import NotPolled, Polled, attr | ||
|
|
||
|
|
||
| class PowerSupply(Controller): | ||
| @attr(Polled(period=0.5), units="V") | ||
| async def voltage(self) -> float: | ||
| """Read every half second, because the device changes it.""" | ||
| return float(await self._conn.query("V?")) | ||
|
|
||
| @attr | ||
| async def serial_number(self) -> str: | ||
| """Read once, when the controller connects.""" | ||
| return await self._conn.query("*IDN?") | ||
|
|
||
| @attr(NotPolled()) | ||
| async def last_error(self) -> str: | ||
| """Never read on a schedule - only when something asks for it.""" | ||
| return await self._conn.query("ERR?") | ||
| ``` | ||
|
|
||
| A bare `@attr` means read once, at connect - the same default a bare | ||
| `getter=` has. See [](./update-attributes-from-device.md) for the whole picture, | ||
| including devices that push values at you rather than being polled. | ||
|
|
||
| ## Commands | ||
|
|
||
| PyTango's `@command` and FastCS's `@command` line up directly, including typed | ||
| arguments and return values: | ||
|
|
||
| ```python | ||
| from fastcs.methods import command | ||
|
|
||
|
|
||
| class PowerSupply(Controller): | ||
| @command() | ||
| async def reset(self) -> None: | ||
| """Return the supply to its power-on state.""" | ||
| await self._conn.send("*RST") | ||
| ``` | ||
|
|
||
| See [](./typed-commands.md) for arguments and return values, and which | ||
| transports can serve them. | ||
|
|
||
| ## When not to use `@attr` | ||
|
|
||
| `@attr` is the simple case: one attribute, one device call, known at the time | ||
| you write the class. It is sugar over the procedural form, and there are two | ||
| other spellings for when it stops fitting: | ||
|
|
||
| - **The attribute needs more than a getter and a setter** - a shared connection | ||
| object, several attributes built in a loop, values that come from one | ||
| request - build them in `__init__` with `AttrR(getter=...)` / | ||
| `AttrRW(getter=..., setter=...)` directly. `@attr` degrades into exactly that | ||
| form, so nothing is lost by moving. | ||
| - **The device describes itself** - the attributes are discovered by asking the | ||
| device what it has, rather than written out. Declare what your code refers to | ||
| as type hints and let the controller fill them in at initialisation. See | ||
| [](../tutorials/dynamic-drivers.md). | ||
|
|
||
| There is no free-function `attr()` factory: outside a class body, write the | ||
| constructor. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@shihab-dls I'm tempted to copy PyTango in this rather than
@propertyso we don't need to ignore pyright.If we say the name should be
set_voltage, then the decorator sets the setter on the AttrRW, but returns the setter function.That means
controller.voltageis anAttrRW, butcontroller.set_voltageis a method that setscontroller.voltage. What do you think?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.
100%. I'm actually now re-reviewing this after looking at #425, and realized we need to make this change. I'll add this as a comment on that PR.