Skip to content

Add switches and sensors to Litter-Robot#46942

Merged
bdraco merged 6 commits into
home-assistant:devfrom
natekspencer:litterrobot
Feb 24, 2021
Merged

Add switches and sensors to Litter-Robot#46942
bdraco merged 6 commits into
home-assistant:devfrom
natekspencer:litterrobot

Conversation

@natekspencer
Copy link
Copy Markdown
Contributor

Breaking change

Proposed change

Adds switches and sensors to the Litter-Robot platform for a better end user experience.

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

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:

Comment thread homeassistant/components/litterrobot/switch.py Outdated
Comment thread homeassistant/components/litterrobot/switch.py Outdated
Comment thread homeassistant/components/litterrobot/switch.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Please see comments above

@bdraco bdraco added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 23, 2021
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Feb 23, 2021

second opinion request is for #46942 (comment)

@natekspencer
Copy link
Copy Markdown
Contributor Author

Please see comments above

I've resolved the other changes requested

@natekspencer
Copy link
Copy Markdown
Contributor Author

@bdraco, I removed the "night mode" switch, but moved the attributes in question to the vacuum entity.

The reason for removing the switch is that to turn it on, the user needs to have a time component to specify when it should start. So either it defaults to a start time (which I was doing before, but didn't seem intuitive) or a configuration input_datetime, or something similar, needs to be added for better control. Because the sleep mode can be set by calling a command on the vacuum entity which has been documented, I decided that I could probably do away with this particular switch for now until a better solution is available.

@bdraco bdraco self-requested a review February 23, 2021 17:39
Comment thread homeassistant/components/litterrobot/switch.py Outdated
@bdraco bdraco removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 23, 2021
Comment thread tests/components/litterrobot/test_switch.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Should be good to merge after switching the test to comply with https://developers.home-assistant.io/docs/development_testing/#writing-tests-for-integrations

@natekspencer natekspencer requested a review from bdraco February 23, 2021 20:20
Comment thread tests/components/litterrobot/conftest.py Outdated
@natekspencer natekspencer requested a review from bdraco February 23, 2021 21:08
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@bdraco bdraco merged commit b8f7bc1 into home-assistant:dev Feb 24, 2021
@joelmoses
Copy link
Copy Markdown

Congrats, @natekspencer. I look forward to using the native integration and not HACS. :)

@natekspencer
Copy link
Copy Markdown
Contributor Author

Thanks, @bdraco and @joelmoses! I'm looking forward to having more people use this and see what changes can be made in the future!

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.

Please address the comments in a new PR. Thanks!

def device_state_attributes(self):
"""Return device specific state attributes."""
return {
"cycle_count": self.robot.cycle_count,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can these attributes be separate sensor entities instead? If a measurement is relevant on its own we want it to be a separate entity.

Comment thread homeassistant/components/litterrobot/switch.py
[sleep_mode_start_time, sleep_mode_end_time] = [None, None]

if self.robot.sleep_mode_active:
sleep_mode_start_time = dt_util.as_local(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Times in state attributes must be absolute UTC time.

Comment thread tests/components/litterrobot/conftest.py
Comment thread tests/components/litterrobot/conftest.py
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 26, 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.

5 participants