-
Notifications
You must be signed in to change notification settings - Fork 38
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
[2.0] Add Floors, Labels, and additional device properties #290
Conversation
Fixes #249 |
consider also adding state_class |
Coverage Report
|
@legrego good to go! |
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.
Thanks for your patience, I know I've been slow with reviews lately. Code-review only, will test locally for the next iteration
"unit_of_measurement": str(entity.unit_of_measurement), | ||
"area.id": entity_area.id if entity_area else None, | ||
"area.name": entity_area.name if entity_area else None, | ||
"class": entity_capabilities.get("state_class"), |
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.
question Why did you choose class
over state_class
for the property name?
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.
value is technically state.value so I was thinking that just class might make sense here, I think in the other PR i lift device_class to be device.class. Happy to just call it state_class though.
device_additions = {
"class": entity.device_class or entity.original_device_class,
"class": entity_capabilities.get("state_class"),
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.
I slightly prefer state_class
. Leaving this as entity.class
gives me the impression that this describes the class of the entity
, as opposed to the entity's state
.
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.
are you okay with me merging this PR and making new open issues for this and moving the Area and I'll incorporate those into my next PR?
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 that would be fine with me
@@ -147,6 +162,17 @@ | |||
"type": "keyword" | |||
} | |||
} | |||
}, | |||
"floor": { |
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.
I'm torn about placing floor
here, as it's technically an attribute of the area
. I think it's easier to consume where you have it, but the pedant in me says it doesn't belong here.
Let's leave it the way you have it for now.
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.
Are you sure? I'd be happy to move it under the Area
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.
Let's move it under area
for both entity.area
and device.area
, if you don't mind.
@legrego 3 open discussions but incorporated the rest of the feedback |
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.
LGTM, with naming nits for follow-up
device = self._registry_device.async_get(entity.device_id) | ||
if device.area_id: | ||
device_area = self._registry_area.async_get_area(device.area_id) | ||
entity_label = self.async_get_entity_labels(entity) |
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.
entity_label = self.async_get_entity_labels(entity) | |
entity_labels = self.async_get_entity_labels(entity) |
if device is None: | ||
LOGGER.debug("Device not found for entity: %s", entity_id) | ||
|
||
device_label = self.async_get_device_labels(device) |
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.
device_label = self.async_get_device_labels(device) | |
device_labels = self.async_get_device_labels(device) |
entity, | ||
entity_area, | ||
entity_floor, | ||
entity_label, |
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.
entity_label, | |
entity_labels, |
device, | ||
device_area, | ||
device_floor, | ||
device_label, |
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.
device_label, | |
device_labels, |
"unit_of_measurement": str(entity.unit_of_measurement), | ||
"area.id": entity_area.id if entity_area else None, | ||
"area.name": entity_area.name if entity_area else None, | ||
"class": entity_capabilities.get("state_class"), |
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 that would be fine with me
Add floors support for devices
Add labels support for devices and entities
Add unit_of_measure for entities
Add class for devices (device_class, ex.
timestamp
) and entities (state_class)Switch dynamic mode from strict to false to ignore extra fields present in the documents instead of failing to index
Bumps Python to 3.12 and minimum HA version to 2024.4.4
Ready to go on everything except updating doc_creator and doc_publisher tests as I want to wait until #272 merges before updating those tests
Resolves #249
Resolves #299