Skip to content

attributes: @attr decorator sugar over the getter/setter constructors - #423

Merged
shihab-dls merged 4 commits into
refactorfrom
refactor-issue-397
Sep 4, 2026
Merged

attributes: @attr decorator sugar over the getter/setter constructors#423
shihab-dls merged 4 commits into
refactorfrom
refactor-issue-397

Conversation

@coretl

@coretl coretl commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Closes #397

An attribute can now be declared by decorating the method that reads it, with @x.setter for the writer half - the PyTango hello-world, written over the getter=/setter= constructors from #392 rather than beside them.

class PowerSupply(Controller):
    @attr(Polled(period=0.5), units="V")
    async def voltage(self) -> float:  # pyright: ignore[reportRedeclaration]
        """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}")

Scope

  • @attr is a decorator only, in two forms: bare @attr and parameterised @attr(Polled(period=0.5), units="V"). Getter only builds an AttrR; getter + @x.setter builds an AttrRW. There is no @attr_r/@attr_rw, no free-function attr() factory, and no write-only decorator - AttrW(setter=…) stays longhand.
  • The datatype is the getter's return annotation, unwrapping Update[T], and supports the Array1D/Table spellings. Not annotating it fails at decoration with a message naming the getter.
  • The getter's docstring becomes the description - its first paragraph, as one line. An explicit description= wins.
  • Decorator keyword arguments are the attribute's metadata, typed Unpack[Meta] (the superset, since the datatype is not known until the getter is read) and runtime-validated against the datatype by the constructor, so @attr(precision=3) on a -> str getter raises naming the field, datatype and attribute.
  • The optional leading positional is a schedule - the same Polled/NotPolled objects the procedural form wraps its getter in, per ADR 0018's table. A bare @attr is read once at connect, exactly as a bare getter= is.
  • Binding follows @command/@scan: the class body holds an UnboundAttr declaration, and BaseController._bind_attrs builds a fresh AttrR/AttrRW per instance with the getter and setter bound to it. Nothing is deepcopied from a class-scope prototype, so two controllers never share an attribute or write to each other's device.
  • Name clashes already raise: a decorated attribute is added during __init__, so a later self.voltage = AttrR(...) hits the existing _check_for_name_clash. A matching voltage: AttrR[float] hint is validated against the decorated attribute by the existing hinted-attribute check.
  • New docs/how-to/fastcs-for-pytango-users.md, pairing each spelling with its PyTango equivalent and saying when to reach for @attr rather than the constructor or (when it lands, ControllerFiller — declarative/procedural split #394) the filler.

Instructions to reviewer on how to test:

  1. uv run pytest tests/test_attr_decorator.py -v
  2. Write a two-attribute controller with @attr, serve it over EPICS CA, and confirm the PVs carry the docstring as DESC and the decorator's units/precision.

Checks for reviewer

  • Would the PR title make sense to a user on a set of release notes
  • A read-write @attr needs # pyright: ignore[reportRedeclaration] on the getter. This is the one wart, and it is inherent to the @x.setter spelling rather than to this implementation: type checkers special-case the builtin property and nothing else, so two def voltage in one class body is an error (pyright: obscured by a declaration of the same name; mypy: already defined). I measured the alternatives before choosing:
    • As implemented - UnboundAttr is a plain non-data descriptor whose __get__ is typed to return AttrR[T], and UnboundAttrRW's returns AttrRW[T]. self.voltage.set(...), .readback, .setpoint and the datatype all check correctly at every use site; the cost is one suppression comment per read-write declaration, which the docs page spells out.
    • Deriving UnboundAttr from property silences the diagnostic completely - but pyright then evaluates the whole pair through its property path, and c.voltage comes out as AttrR[Unknown]: .set() is unknown and even the datatype is lost. That trades one comment at the declaration for a cast at every use, which is why I did not take it.
      I have kept the ADR's spelling; say if you would rather have the property derivation, or a different spelling that avoids the collision (@voltage.writer on a differently-named method), and it is a small commit either way.
  • @attr(Polled(period=0.5)), not Polled(0.5). ADR 0018 writes the shorthand Polled(0.5), but Polled's first positional is its getter and period is keyword-only (Polled(protocol.get_temperature, period=0.2) is the procedural form throughout the demo and docs), so Polled(0.5) would silently bind 0.5 as the getter. Rather than reorder Polled's fields and rewrite every procedural call site, @attr takes a bare Polled(period=0.5), and rejects a schedule that already carries a getter. Say if you would rather Polled grew a period-first spelling.
  • The docstring contributes its first paragraph, not the whole thing. A description is the one-line label a transport puts next to the value (CA DESC is 40 characters), so a getter with a summary line and further explanation contributes only the summary. Say if you want the whole docstring.

Notes

🤖 Generated with Claude Code

https://claude.ai/code/session_01SiGhLM9QRKpnQykdmMfLsh


Generated by Claude Code

Adds the `@attr` decorator from ADR 0018: a controller declares an
attribute by decorating the method that reads it, with `@x.setter` for
the writer half, mirroring `@property`.

The datatype comes from the getter's return annotation (unwrapping
`Update[T]`), the getter's docstring summary becomes the description,
and decorator keyword arguments are the attribute's metadata, validated
against the datatype. The optional leading positional is a
`Polled`/`NotPolled` schedule, so the declarative and procedural
spellings share one vocabulary; a bare `@attr` is read once at connect,
as a bare `getter=` is.

Binding follows `@command`/`@scan`: the class body holds an
`UnboundAttr` declaration and each controller instance binds a fresh
`AttrR`/`AttrRW` of its own, so nothing is deepcopied from a class-scope
prototype. `UnboundAttr` is a non-data descriptor so that a decorated
attribute reads as the attribute it becomes rather than the
declaration - `UnboundAttrRW` carries the `AttrRW` typing.

Adds the "FastCS for PyTango users" docs page.

Closes #397

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SiGhLM9QRKpnQykdmMfLsh
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: ea72352b-70d1-4ee6-8c89-fd88516fbd78

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.12281% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.63%. Comparing base (e73453b) to head (a5b7bf7).
⚠️ Report is 5 commits behind head on refactor.

Files with missing lines Patch % Lines
src/fastcs/attributes/attr_decorator.py 99.02% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           refactor     #423      +/-   ##
============================================
+ Coverage     91.25%   92.63%   +1.38%     
============================================
  Files            72       70       -2     
  Lines          2892     3298     +406     
============================================
+ Hits           2639     3055     +416     
+ Misses          253      243      -10     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shihab-dls shihab-dls left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pushed a few changes. Now happy with the final shape, so approving on my end.

@shihab-dls
shihab-dls merged commit fc74689 into refactor Sep 4, 2026
11 checks passed
@shihab-dls
shihab-dls deleted the refactor-issue-397 branch September 4, 2026 12:57
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:

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.

@shihab-dls I'm tempted to copy PyTango in this rather than @property so 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.voltage is an AttrRW, but controller.set_voltage is a method that sets controller.voltage. What do you think?

@shihab-dls shihab-dls Sep 6, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants