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.

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

Integrate climate with the smartthinq platform. #27

Merged
merged 3 commits into from
Aug 19, 2019
Merged

Integrate climate with the smartthinq platform. #27

merged 3 commits into from
Aug 19, 2019

Conversation

dermotduffy
Copy link
Contributor

Caution: I can only partially test this, as I do not have any LG HVAC gear. It appears to do the right thing as far as the point of device creation logic -- and the right warning messages appear to get printed. This will support both the old style (printing warning messages) and the new style.

You may want to test this yourself.

Also took the liberty of updating the README. Happy to remove the dishwasher image if you'd rather keep it clean.

Copy link
Owner

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Great; thanks! Here are a few suggestions.

climate.py Outdated Show resolved Hide resolved
climate.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@dermotduffy
Copy link
Contributor Author

Thanks for the great suggestions. All implemented.

@sampsyo
Copy link
Owner

sampsyo commented Aug 19, 2019

Looking great; thank you!!

I gave this a shot, and it looks like we might have one small thing to clean up: the component fails to load if the smartthinq config doesn't exist at the top level. It would be great to fail more gracefully when this happens so it's still possible to use the climate-specific config instead (although it's depreciated). Specifically, I get this message in my log at startup:

Traceback (most recent call last):
  File "/Users/asampson/Library/Python/3.7/lib/python/site-packages/homeassistant/setup.py", line 172, in _async_setup_component
    component.setup, hass, processed_config  # type: ignore
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/asampson/.homeassistant/custom_components/smartthinq/__init__.py", line 37, in setup
    refresh_token = config[DOMAIN].get(CONF_TOKEN)
KeyError: 'smartthinq'

And also:

2019-08-18 22:37:33 ERROR (MainThread) [homeassistant.setup] Unable to prepare setup for platform smartthinq.climate: Unable to set up component.

And the UI reports a message like this:

The following components and platforms could not be set up:

smartthinq
smartthinq.climate

Please check your config.

So maybe we just have to be tolerant of the missing config[DOMAIN]?

@dermotduffy
Copy link
Contributor Author

dermotduffy commented Aug 19, 2019

Hmm. Is it possible you are testing the version without this PR? I specifically tested the configuration you describe. Here it is working:

2019-08-18 19:47:47 WARNING (SyncWorker_32) [custom_components.smartthinq] Direct use of the smartthinq components without a toplevel smartthinq platform configuration is deprecated. Please use a top-level smartthinq platform instead. Please see https://github.com/sampsyo/hass-smartthinq/blob/master/README.md . Configuration mapping:
        climate.refresh_token -> smartthinq.token
        climate.country -> smartthinq.region
        climate.language -> smartthinq.language

Configuration used:

#smartthinq:
#  token: !secret smartthinq_token
#  region: US
#  language: en-US

climate:
  - platform: smartthinq
    refresh_token: !secret smartthinq_token
    country: US
    language: en-US

The other clue is that your stacktrace line numbers don't match my master repo (and this PR). With this PR, prior to the code shown in your stacktrace, is this block:

 57     if DOMAIN not in config:
 58         LOGGER.warning(DEPRECATION_WARNING)
 59         return True

... i.e. it should not get as far as the stacktrace shows.

Let me know if there's any chance you didn't patch in this full PR when you ran that test?

@sampsyo
Copy link
Owner

sampsyo commented Aug 19, 2019

Aha; you're so right! Sorry about that.

There's another problem, however—here's the traceback:

Traceback (most recent call last):
  File "/Users/asampson/Library/Python/3.7/lib/python/site-packages/homeassistant/helpers/entity_platform.py", line 149, in _async_setup_platform
    await asyncio.wait_for(asyncio.shield(task), SLOW_SETUP_MAX_WAIT)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/tasks.py", line 442, in wait_for
    return fut.result()
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/asampson/.homeassistant/custom_components/smartthinq/climate.py", line 56, in setup_platform
    hass.data[CONF_REGION]
KeyError: 'region'

This happens if neither method specifies a country code. Maybe that line needs to use a .get() as well?

@dermotduffy
Copy link
Contributor Author

My goodness, sorry about that. We'll get there in the end ... please take another look.

@sampsyo
Copy link
Owner

sampsyo commented Aug 19, 2019

Wonderful! That did it. Thanks!!

@sampsyo sampsyo merged commit e25cf5f into sampsyo:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants