Clarify state attributes rules for entity #2437
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/core/entity.md (2)
122-132: Fix grammar and consider adding a code example.
- There's a grammatical error in the text. Please fix:
-If an attributes is not expected +If an attribute is not expected
- Consider adding a code example to illustrate these rules. For instance:
class MyEntity(Entity): @property def extra_state_attributes(self): """Return entity specific state attributes.""" attributes = { "expected_attribute": self._value or None, # Always present "configuration_dependent": "value" # Only when configured } return attributes🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...(A_NNS)
Line range hint
134-139: Consider enhancing the System properties documentation.The section could be more helpful with additional context:
- Add a note explaining that these properties are managed through the entity registry.
- Add a warning about not implementing custom logic for these properties, as they are system-managed.
Example addition:
:::warning These properties are managed by Home Assistant through the entity registry. Integrations should not implement custom logic for these properties as it may conflict with system behavior. :::🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...(A_NNS)
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/core/entity.md
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...
(A_NNS)
🔇 Additional comments (1)
docs/core/entity.md (1)
Line range hint 1-1000: Overall documentation changes look good!
The additions effectively clarify the rules for state attributes and system properties, which aligns well with the PR objectives. The document maintains good structure and readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~131-~131: The plural noun “attributes” cannot be used with the article “an”. Did you mean “an attribute” or “attributes”?
Context: ...None` and remain in the dictionary - If an attributes is not expected, it should not be in th...
(A_NNS)
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count
(MD056, table-column-count)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/core/entity.md (2)
122-132: Enhance the state attributes section with examplesWhile the rules are clear, adding concrete examples would help developers better understand when attributes should be included or excluded from the dictionary. Consider adding examples for:
- Expected attributes (e.g., supported features)
- Attributes that should be None
- Attributes that should not be in the dictionary
Here's a suggested addition after line 132:
- If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. + If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. + + ### Examples + + ```python + @property + def extra_state_attributes(self): + """Return entity specific state attributes.""" + attributes = {} + + # Example 1: Expected attribute based on supported features + if self._attr_supported_features & SUPPORT_BRIGHTNESS: + attributes["brightness"] = self._brightness + + # Example 2: Expected attribute that might be None + if self._attr_supported_features & SUPPORT_COLOR: + attributes["rgb_color"] = self._rgb_color # Could be None + + # Example 3: Conditional attribute that should not be included + # Don't do this: + # if self._is_dimmable: + # attributes["brightness"] = self._brightness + + return attributes + ```
Line range hint
143-401: LGTM with a minor suggestionThe updated sections on entity naming and property implementation are clear and well-documented. The examples effectively demonstrate best practices. Consider adding a note about error handling in the property implementation example to make it more complete.
Add error handling to the example:
def update(self) -> None: """Update entity state.""" try: self._device.update() - except ExampleException: + except ExampleException as ex: if self.available: # Read current state, no need to prefix with _attr_ - LOGGER.warning("Update failed for %s", self.entity_id) + LOGGER.warning("Update failed for %s: %s", self.entity_id, str(ex)) self._attr_available = False # Set property value return + except Exception as ex: + LOGGER.error("Unexpected error updating %s: %s", self.entity_id, str(ex)) + self._attr_available = False + return🧰 Tools
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/core/entity.md(1 hunks)
🔇 Additional comments (1)
docs/core/entity.md (1)
Line range hint 134-141: LGTM!
The new "System properties" section clearly explains that these properties are controlled by Home Assistant and should not be overridden by integrations. The documentation for the enabled property is accurate and helpful.
🧰 Tools
🪛 Markdownlint
120-120: Expected: 4; Actual: 3; Too few cells, row will be missing data
Table column count
(MD056, table-column-count)
| ## State attributes | ||
|
|
||
| Entities can provide additional state attributes besides the built-in ones, to provide further details to its state. | ||
| This should be done by providing a dictionary with keys and values to the `extra_state_attributes` property. | ||
|
|
||
| Providing state attributes comes with the following rules: | ||
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". |
There was a problem hiding this comment.
Should we also indicate our rules for what belongs in state attributes and what should be separate entities and what should be action calls?
There was a problem hiding this comment.
I think that would probably be good although do we really have clear rules?
I think we could do that in a follow-up in that case.
There was a problem hiding this comment.
Frenck once mentioned this in the members channel
Some helpful guidance with that, that I apply (for myself)
- Would someone extract it using a template into an other (binary) sensor -> Not an attribute
- It is essential to automation on? Can it be standalone as its own entity? -> Not an attribute
- It is static, e.g., from configuration; doesn't belong in the state machine -> Not an attribute
- Do we want to track long term stats of the value? -> Not an attribute
Additionally:
- Does is have historical value? If not, exlude it from being recorded.
There was a problem hiding this comment.
I think that makes sense but I'm not sure we can classify it as rules?
Maybe as "guidelines"?
There was a problem hiding this comment.
Oh right. I think that if we are going to talk about state attributes, that we also explain that we are picky in what we want using these guidelines :)
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". | ||
| - If an attribute is expected, but it's not providing a value right now, its value should be `None` and remain in the dictionary. | ||
| - If an attribute is not expected, it should not be in the dictionary. An attribute is expected if supported by the entity, for example as indicated by `supported_features`. |
There was a problem hiding this comment.
I am not really sure what this means and how it would look in practice
There was a problem hiding this comment.
As an example, in statistics the buffer_usage_ratio attribute is only shown if the sensor is configured to handle a buffer size. Otherwise this attribute would be irrelevant hence not showing in that case.
There was a problem hiding this comment.
But how can someone expect it to be there? We're quite bad with describing entities in the docs, let alone describing the state attributes
There was a problem hiding this comment.
This is the developer docs not the user docs. The attribute is expected if it's part of a supported feature and that feature is supported or if it's an extra state attribute the attribute is expected if the device has support for that attribute.
| Providing state attributes comes with the following rules: | ||
|
|
||
| - If an attribute is expected, it should be in the dictionary. Attributes should not "come and go". | ||
| - If an attribute is expected, but it's not providing a value right now, its value should be `None` and remain in the dictionary. |
There was a problem hiding this comment.
I think the first 2 rules are quite similar. I also think we should explain what happens when for example the entity is unavailable. What should we do then?
There was a problem hiding this comment.
We could perhaps combine the first two as they talk about the same thing.
I'm not sure what you mean by unavailable? Should there be any special consideration for it?
There was a problem hiding this comment.
If an entity is unavailable, does it still read the extra state attributes property when writing state? And if so, should everything be set to None?
There was a problem hiding this comment.
State attributes are only set if the state of the entity is not unavailable. This is part of the base entity logic. Integrations should not adjust any logic concerning availability and state attributes.
Proposed change
Clarify the rules regarding additional state attributes as decided in home-assistant/architecture#680
Type of change
Additional information
Summary by CodeRabbit
has_entity_nameproperty.