Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve nest setup error handling #44385

Merged
merged 7 commits into from
Dec 28, 2020
Merged

Improve nest setup error handling #44385

merged 7 commits into from
Dec 28, 2020

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Dec 19, 2020 •
edited
Loading

Proposed change

Make the nest integration handle errors a bit cleaner by logging once to error logs. This is meant to make the nest integration quieter, as one of the last remaining items on the silver integration quality scale.

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:

allenporter/python-google-nest-sdm@v0.2.1...v0.2.2

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:

@project-bot project-bot bot added this to Needs review in Dev Dec 19, 2020
@probot-home-assistant
Copy link

Hey there @awarecan, mind taking a look at this pull request as its been labeled with an integration (nest) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Dec 19, 2020
j-stienstra
j-stienstra reviewed Dec 21, 2020
View reviewed changes
Copy link
Contributor

@j-stienstra j-stienstra left a comment

Choose a reason for hiding this comment

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

Hi,

Very new here, just submitted my first PR, so bear with me if I suggest pointless changes.
I just have two minor suggestions, the rest looks good to me.

tests/components/nest/test_init_sdm.py Outdated Show resolved Hide resolved
tests/components/nest/test_init_sdm.py Outdated Show resolved Hide resolved
@allenporter
Copy link
Contributor Author

Ready for another look.

j-stienstra
j-stienstra approved these changes Dec 21, 2020
View reviewed changes
tests/components/nest/test_init_sdm.py Outdated Show resolved Hide resolved
@frenck frenck changed the title Improve nest setup error handling with notificaitons Improve nest setup error handling with notifications Dec 24, 2020
MartinHjelmare
MartinHjelmare requested changes Dec 26, 2020
View reviewed changes
homeassistant/components/nest/__init__.py Outdated
@@ -183,19 +190,40 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
data=entry.data,
)
)
except ConfigurationException as err:
persistent_notification.async_create(
Copy link
Member

@MartinHjelmare MartinHjelmare Dec 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We prefer to not add integration specific notifications. We can consider adding a common default notification on entry setup failure.

Copy link
Contributor Author

@allenporter allenporter Dec 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I will update this to just log errors for now then. My real goal here is to move up the integration quality scale and meet the criteria of "Log a warning once when unavailable, log once when reconnected" from https://developers.home-assistant.io/docs/integration_quality_scale_index/#silver-%F0%9F%A5%88

(1) I will think more about default notifications. I imagine that will take more thought and feedback to work through to understand the existing preferences. What is the best forum for getting feedback on a "design" for that? Wasn't clear if this rises to the level of ADR, or just a PR, and my assumption is that discussing over code review may be too low level.

(2) What is the recommended way to handle logging once? The approach i've commonly seen is to store global state in hass.data to track this (or via some wrapper class stored in hass.data), but wanted to ask before adding that, as it is not super satisfying.

Copy link
Member

Choose a reason for hiding this comment

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

  1. We can open an architecture issue and/or post the question on discord.
  2. Yeah, store a boolean somewhere and use it to check if we've already logged.

Dev automation moved this from By Code Owner to Review in progress Dec 26, 2020
@allenporter allenporter changed the title Improve nest setup error handling with notifications Improve nest setup error handling Dec 27, 2020
@allenporter allenporter self-assigned this Dec 27, 2020
allenporter and others added 5 commits December 27, 2020 09:43
@allenporter
Improve error handling user experience
3539d20 
This is meant to make the nest integration quieter.  Exceptions are handled
by showing notifications to the user with additional error detail.
@allenporter
Add clarification on where an error message comes from
8ab6a9e
@allenporter @j-stienstra
Add assert for no existing notification at test setup
dc71dd9 
Co-authored-by: j-stienstra <65826735+j-stienstra@users.noreply.github.com>
@allenporter
Fix indent in patch
4055276
@allenporter
Remove persistent notification based on PR feedback.
874f09d
MartinHjelmare
MartinHjelmare approved these changes Dec 28, 2020
View reviewed changes
Copy link
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.

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Dec 28, 2020
@MartinHjelmare
Copy link
Member

Maybe update the PR description?

@allenporter
Copy link
Contributor Author

Maybe update the PR description?

Done.

@allenporter allenporter merged commit 71af0fa into home-assistant:dev Dec 28, 2020
Dev automation moved this from Reviewer approved to Done Dec 28, 2020
KJonline pushed a commit to Pyhass/core that referenced this pull request Dec 28, 2020
Merge branch 'dev' of https://github.com/home-assistant/core into dev
15965c9 
* 'dev' of https://github.com/home-assistant/core:
  Fix Tasmota device triggers (home-assistant#44574)
  Improve nest setup error handling (home-assistant#44385)
  Tado: add full list of devices (home-assistant#44475)
  Simplify nest event handling (home-assistant#44367)
  Add explicit support for Luxembourg Smarty meter in dsmr integration (home-assistant#43975)
  Fully remove Avri integration (home-assistant#44478)
  Add Olivia voice to Amazon Polly TTS (home-assistant#44513)
  Bump pydeconz to version 77 (home-assistant#44514)
  Add album art support in the mpd component (home-assistant#44527)
  Fix falsey comparisons in NWS weather (home-assistant#44486)
  Upgrade huawei-lte-api to 1.4.17 (home-assistant#44499)
  Update README.rst to avoid redirects (home-assistant#44519)
  Handle missing Somfy devices during update (home-assistant#44425)
  Convert mpd component to use the async MPDClient (home-assistant#44384)
  Bump pywemo to 0.5.6 (home-assistant#44440)
  Support auto as Dyson fan on device state (home-assistant#44472)
  [ci skip] Translation update
  Remove useless async_add_executor_job (home-assistant#44496)
  Fix the docstring in type_fans.py (home-assistant#44511)
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2020
@allenporter allenporter deleted the nest_config_error branch December 30, 2020 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@j-stienstra j-stienstra j-stienstra approved these changes

@frenck frenck Awaiting requested review from frenck

Assignees

@allenporter allenporter

Labels
by-code-owner cla-signed code-quality has-tests integration: nest small-pr PRs with less than 30 lines.
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@allenporter @MartinHjelmare @frenck @j-stienstra @awarecan @homeassistant