Skip to content

Adjust litterrobot tests and code to match guidelines#47060

Merged
MartinHjelmare merged 6 commits into
home-assistant:devfrom
natekspencer:litterrobot
Mar 6, 2021
Merged

Adjust litterrobot tests and code to match guidelines#47060
MartinHjelmare merged 6 commits into
home-assistant:devfrom
natekspencer:litterrobot

Conversation

@natekspencer
Copy link
Copy Markdown
Contributor

@natekspencer natekspencer commented Feb 25, 2021

Proposed change

Adjust the tests and code to match suggestions from previous PR reviews.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@natekspencer
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare, I've made most of the changes you requested. A couple responses to your comments that I haven't addressed yet:

(#45886 (comment)): The entity_type that is passed in is controlled, so there isn't a risk of a trailing space at the moment.

(#46942 (comment)): Theoretically these can be separate sensors, but I don't know how relevant they are on their own. Sometimes it is interesting to see, just to know what is the basis for the "Waste Drawer" gauge calculation. Should they still be separated and/or removed?

(#46942 (comment)): Please see #46942 (comment) for previous conversation on this and let me know how to proceed.

@MartinHjelmare
Copy link
Copy Markdown
Member

Times in state attributes should be UTC, there is no exception.

Comment thread homeassistant/components/litterrobot/hub.py Outdated
Comment thread tests/components/litterrobot/test_init.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Feb 26, 2021

The entity_type that is passed in is controlled, so there isn't a risk of a trailing space at the moment.

If there's always an entity type passed in we should remove the other case.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Feb 26, 2021

Theoretically these can be separate sensors, but I don't know how relevant they are on their own. Sometimes it is interesting to see, just to know what is the basis for the "Waste Drawer" gauge calculation. Should they still be separated and/or removed?

What do these attributes measure?

As an example, a temperature measurement from a temperature sensor is relevant on its own, but a brightness measurement from a lamp isn't relevant without knowing if the lamp is on or off.

@natekspencer
Copy link
Copy Markdown
Contributor Author

The entity_type that is passed in is controlled, so there isn't a risk of a trailing space at the moment.

If there's always an entity type passed in we should remove the other case.

Now I see what you are referring to. That was a relic of a previous version of entity_types in HACS that I hadn't removed. Thanks for the catch.

@natekspencer
Copy link
Copy Markdown
Contributor Author

Times in state attributes should be UTC, there is no exception.

Is there a way to show only the time portion, whilst storing the absolute UTC time? The reason I ask is that the Litter-Robot API returns an epoch time for when the "sleep mode start time" is first set and is not updated unless the start time is manually changed. So if a user set that a month ago, there is now a month old "sleep mode start time", which makes zero sense to a user.

Sure, I could do some "magic" and update the date to be today's date. But then there is another question: do I always show the next start time, or do I show the previous start time while the unit is "sleeping" since it is an 8-hour window?

Also, as an end user of the robot, there are really only two pertinent data points associated with "sleep mode":

  1. is sleep mode on or off
  2. if on, what time is sleep mode set for

If there is a date component displayed, the user now has to decipher what it means to gain value out of it, which seems counter-intuitive to what home automation is all about.

Again, I look back on the input_datetime helper that only has a time component for how the history is tracked on it and it is precisely the functionality that makes sense for this data point. When I set a time on that helper, it doesn't care about a date and I only see state changes for things that make sense. There isn't a way to create an input_datetime helper through an integration is there? (I tried and got errors.) That would actually solve this altogether...

@MartinHjelmare
Copy link
Copy Markdown
Member

If the sleep mode measurements are supposed to show the start of the current sleep and the future end of the current sleep, I'd expect the time measurement to include the correct date besides the time.

Consider breaking out these measurements to sensors with device class timestamp. Timestamp sensors can be shown as relative time when using the Lovelace frontend entities card.

https://developers.home-assistant.io/docs/core/entity/sensor#available-device-classes

@MartinHjelmare MartinHjelmare changed the title Adjust tests and code to match guidelines Adjust litterrobot tests and code to match guidelines Feb 27, 2021
@natekspencer
Copy link
Copy Markdown
Contributor Author

Alright, @MartinHjelmare, I moved those attributes to their own timestamp sensor with appropriate absolute UTC time and removed the other attributes on the waste drawer sensor per your guidelines!

Comment thread homeassistant/helpers/icon.py Outdated
Comment thread homeassistant/components/litterrobot/sensor.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@natekspencer
Copy link
Copy Markdown
Contributor Author

Thanks!

Thanks for all your help, @MartinHjelmare!

@natekspencer
Copy link
Copy Markdown
Contributor Author

@bdraco, part of the changes here resolve the issue mentioned in #47514, can we merge this pull request or should I create a different one only addressing/fixing that issue?

Comment thread homeassistant/components/litterrobot/hub.py
@MartinHjelmare MartinHjelmare merged commit e905223 into home-assistant:dev Mar 6, 2021
@MartinHjelmare MartinHjelmare added this to the 2021.3.3 milestone Mar 6, 2021
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants