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

Make Netatmo able to discover both Weather station and Health Coach #20274

Merged
merged 6 commits into from Feb 17, 2019

Conversation

Projects
None yet
6 participants
@msvinth
Copy link
Contributor

msvinth commented Jan 20, 2019

Description:

Netatmo is now able to discover both Weather Station and Health Coach devices at the same time.
Before it would only discover one of the types. If you had both types not all devices would be found.

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

@msvinth msvinth force-pushed the msvinth:netatmo_betterdiscovery branch from 557b0ba to 5e2bfc6 Jan 22, 2019

@msvinth msvinth referenced this pull request Feb 2, 2019

Closed

Fix netatmo sensor home-coach error #20627

3 of 9 tasks complete

@msvinth msvinth force-pushed the msvinth:netatmo_betterdiscovery branch from 5e2bfc6 to 697ad5d Feb 4, 2019

@amelchio
Copy link
Contributor

amelchio left a comment

Looks good, I have just a few comments.

Note that a merge conflict must be resolved.

Show resolved Hide resolved homeassistant/components/netatmo/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/netatmo/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/netatmo/sensor.py Outdated
Show resolved Hide resolved homeassistant/components/netatmo/sensor.py Outdated

@msvinth msvinth force-pushed the msvinth:netatmo_betterdiscovery branch from 697ad5d to 9d1ee2f Feb 13, 2019

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented Feb 14, 2019

Please do not force-push after a review, it makes the reviewer start from scratch.

@msvinth

This comment has been minimized.

Copy link
Contributor Author

msvinth commented Feb 14, 2019

Please do not force-push after a review, it makes the reviewer start from scratch.

Sorry, I was not aware of that.

if module_name in not_handled:
not_handled.remove(module_name)
if module_name not in handled:
handled.append(module_name)

This comment has been minimized.

Copy link
@amelchio

amelchio Feb 16, 2019

Contributor

These nested ifs are quite complex. Could something like this work instead?

all_classes = [pyatmo.WeatherStationData, pyatmo.HomeCoachData]
not_handled = {}
# ...
if module_name not in data.get_module_names():
    not_handled[module_name] = not_handled[module_name]+1 if module_name in not_handled else 1
# ...
for module_name, count in not_handled.items():
    if count == len(all_classes):
        _LOGGER.error('Module name: "%s" not found', module_name)

This comment has been minimized.

Copy link
@msvinth

msvinth Feb 16, 2019

Author Contributor

Applied your suggestion

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented on homeassistant/components/netatmo/sensor.py in a4b3543 Feb 16, 2019

Move the import back into a method, having it global will fail if REQUIREMENTS are not already installed.

@msvinth

This comment has been minimized.

Copy link
Contributor Author

msvinth commented Feb 16, 2019

Move the import back into a method, having it global will fail if REQUIREMENTS are not already installed.

Moved back again

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented Feb 17, 2019

Looks good 👍. Please also make a PR for the documentation, mentioning the new health_idx module name .

@amelchio amelchio merged commit df59854 into home-assistant:dev Feb 17, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on netatmo_betterdiscovery at 92.804%
Details

@wafflebot wafflebot bot removed the in progress label Feb 17, 2019

@msvinth msvinth referenced this pull request Feb 17, 2019

Merged

Document Netatmo sensor health_idx #8613

0 of 2 tasks complete

@balloob balloob referenced this pull request Mar 6, 2019

Merged

0.89.0 #21712

@erikproper

This comment has been minimized.

Copy link

erikproper commented Mar 14, 2019

It now indeed discovers my Netatmo Weather station and associated modules, as well as one of my two Home Coaches. However ... only one of the two ...

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented Mar 14, 2019

@erikproper Please file a separate issue.

@erikproper

This comment has been minimized.

Copy link

erikproper commented Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.