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

Add Elgato Avea integration #24281

Merged
merged 22 commits into from Jul 23, 2019

Conversation

@pattyland
Copy link
Contributor

commented Jun 3, 2019

Description:

Add new integration with the Elgato Avea light bulb.

Related issue (if applicable): n/a

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9559

Example entry for configuration.yaml (if applicable):

light:
  - platform: avea

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.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

pattyland added some commits Jun 3, 2019

@homeassistant

This comment has been minimized.

Copy link

commented Jun 3, 2019

Hi @pattyland,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

CODEOWNERS Show resolved Hide resolved
def __init__(self, light):
"""Initialize an AveaLigh."""
self._light = light
self._name = light.get_name()

This comment has been minimized.

Copy link
@OttoWinter

OttoWinter Jun 3, 2019

Member

This is an I/O call, right?

The problem with this will be that if for some reason the device is not reachable the setup will fail.

Get all data in setup_platform, and if something fails due to an exception do raise PlatformNotReady to indicate to the HA core that it will be re-tried later.

This comment has been minimized.

Copy link
@pattyland

pattyland Jun 4, 2019

Author Contributor

Like that 89b69cc? Sorry if this is complete bullshit, I'm a complete newbie to python and ha development

Edit: As flake8 pointed out I missed an Import and should make the try/catch block better: bfa7e7d

homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved

pattyland and others added some commits Jun 4, 2019

Improved readability
Co-Authored-By: Otto Winter <otto@otto-winter.com>
Update requirements_all.txt
Add requirements_all.txt file
@homeassistant

This comment has been minimized.

Copy link

commented Jun 5, 2019

Hi @somuelle-tmt,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

pattyland added some commits Jun 5, 2019

for bulb in nearby_bulbs:
bulb.get_name()
bulb.get_brightness()
except Exception:

This comment has been minimized.

Copy link
@OttoWinter

OttoWinter Jun 6, 2019

Member

Could you make this except type more specific? Usually catching a generic Exception is discouraged. If this uses python's standard APIs, you can probably also use OSError.

This comment has been minimized.

Copy link
@pattyland

pattyland Jun 10, 2019

Author Contributor

So this component is using the python avea lib which has no own exception implemented. But it depends on bluepy which has an own class BTLEException
Should I use this Exception instead of OSError?

pattyland and others added some commits Jun 6, 2019

Unnecessary calculation removed
Co-Authored-By: Otto Winter <otto@otto-winter.com>
@pattyland

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@OttoWinter Do you see anything else I could improve? 🙃

homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved
homeassistant/components/avea/light.py Outdated Show resolved Hide resolved

@home-assistant home-assistant deleted a comment from homeassistant Jul 11, 2019

@MartinHjelmare MartinHjelmare changed the title Adds Elgato Avea integration Add Elgato Avea integration Jul 11, 2019

pattyland added some commits Jul 17, 2019

@MartinHjelmare
Copy link
Member

left a comment

Nice!

@MartinHjelmare MartinHjelmare added this to Reviewer approved in Dev Jul 23, 2019

@MartinHjelmare MartinHjelmare merged commit 4c067ec into home-assistant:dev Jul 23, 2019

9 checks passed

CI Build #20190718.46 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python35) Tests PyTest Python35 succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA

Dev automation moved this from Reviewer approved to Done Jul 23, 2019

@pattyland pattyland deleted the pattyland:platform-avea branch Aug 6, 2019

@balloob balloob referenced this pull request Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.