Use EntityDescription - synology_dsm #55407
Conversation
cdce8p
added
the
project-code_style
|
Hey there @hacf-fr, @Quentame, @mib1185, mind taking a look at this pull request as it has been labeled with an integration ( |
I don't think it makes much sense here to move them. Aside from the switch description, most are imported in Sometimes a |
cdce8p
added
the
smash
| self._attr_unique_id: str = ( | ||
| f"{api.information.serial}_{description.api_key}:{description.key}" | ||
| ) | ||
| self._attr_extra_state_attributes = {ATTR_ATTRIBUTION: ATTRIBUTION} |
There was a problem hiding this comment.
This is static, move it to a class attribute
| @@ -604,51 +598,25 @@ async def async_update(self, now: timedelta | None = None) -> None: | |||
| class SynologyDSMBaseEntity(CoordinatorEntity): | |||
| """Representation of a Synology NAS entry.""" | |||
|
|
|||
| entity_description: SynologyDSMEntityDescription | |||
| unique_id: str | |||
| _attr_extra_state_attributes = {ATTR_ATTRIBUTION: ATTRIBUTION} | |||
There was a problem hiding this comment.
This is a bit dangerous. If one entity instance would update this dict all instances of the class would be updated. It's safer to define mutable objects as instance attributes.
There was a problem hiding this comment.
True, it might be dangerous if unaccounted for. Although, we define other attributes also as statics. They can just as easily be overwriting. Mutable or not shouldn't make a difference here.
At the moment, defining the attribution as static is quite common (14 places). So too is _attr_should_poll, _attr_entity_registry_enabled_default, and to a lesser extend _attr_icon or even _attr_native_unit_of_measurement.
--
I'm not fully against changing them, only I think we should decide it for every attribute then and not just of extra_state_attributes.
@ludeeus What do you think?
There was a problem hiding this comment.
The important point here is that we set a class attribute with a mutable object. The other examples you give are not mutable. Those are not a problem.
It's not a problem if an instance overwrites the attribute. That won't affect the other instances. The problem comes when one instance mutates a mutable object stored in a class attribute. Then all instances of that class will see the change. That is probably a bug.
There was a problem hiding this comment.
Got it. I'll open a PR shortly to change all mutable class attributes to instance once. (At least all I can find.)
There was a problem hiding this comment.
It's currently not used/set in any of the subclasses for this integration, and as far as I know, every other integration that actively uses attributes resets (replaces) it in their update method or implement their own property.
But I do see your point, it's easy to make a mistake here in the future.
Proposed change
Update
synology_dsmto useEntityDescriptionfor sensor, binary_sensor, and switch metadata.-> #53201
Type of change
Checklist
black --fast homeassistant tests)