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 native ESPHome API component #19334

Merged
merged 24 commits into from Dec 17, 2018

Conversation

Projects
None yet
5 participants
@OttoWinter
Copy link
Contributor

OttoWinter commented Dec 15, 2018

Description:

This is a MVP for a native API for esphomelib.
Actually it's even less than MVP: there's no way to configure it since this will be a config-flow only integration (only way to configure it with this PR is by editing config entries storage).

This PR includes the basic functionality for connecting to the API and a sample sensor platform for seeing how platforms would look like with the helpers.

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

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 user exposed functionality or configuration variables are added/changed:

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.
  • New files were added to .coveragerc.

OttoWinter added some commits Dec 13, 2018

MVP
Show resolved Hide resolved .coveragerc Outdated
Show resolved Hide resolved CODEOWNERS Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/esphomelib/__init__.py Outdated

@OttoWinter OttoWinter force-pushed the OttoWinter:esphomelib branch from aae5917 to 7a1f88d Dec 15, 2018

OttoWinter added some commits Dec 15, 2018

OttoWinter added some commits Dec 16, 2018

@OttoWinter

This comment has been minimized.

Copy link
Contributor

OttoWinter commented Dec 16, 2018

Oh and for documentation: There's no way to configure this component at the current state (if possible, I would like it to be only configurable through config flow, as an "import" step is kinda hard/impossible here - users can use test.local but also 192.168.178.42, there's no way to check if an imported config entry already exists).

So is it okay to add docs once this PR is done and I add the config flow PR?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 16, 2018

We probably want to add the config flow with tests in this PR, to make it usable.

I think the code looks very good now.

@OttoWinter OttoWinter requested a review from home-assistant/core as a code owner Dec 16, 2018

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 16, 2018

Can we rename this component to esphome ?

OttoWinter added some commits Dec 16, 2018

@OttoWinter OttoWinter changed the title Add native esphomelib API component Add native ESPHome API component Dec 16, 2018

OttoWinter added some commits Dec 16, 2018

OttoWinter added some commits Dec 16, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

We're missing a strings.json file in the package. This file will be the template for the translation json files.

@OttoWinter

This comment has been minimized.

Copy link
Contributor

OttoWinter commented Dec 16, 2018

Ah sorry about that, didn't read the guide properly 😅

I moved the .translations/en.json to strings.json and ran the script/translations_develop script. That script copied the file back (and changed the tellduslive and zha translations). Should I have run the script or not?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 16, 2018

The translation upload script will be run on each dev branch build (not PRs) by travis, as part of the deploy step (if I understand things correctly). So we don't really need to include the translation files in PRs. But to be able to test locally properly it's nice to at least have the English translation file generated.

We probably shouldn't include files from other components in this PR.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Some logic paths of the config flow are not tested:

(home-assistant) m:~/Dev/home-assistant$ pytest --cov-report term-missing --cov=homeassistant.components.esphome.config_flow tests/components/esphome/test_config_flow.py 
Test session starts (platform: linux, Python 3.6.5, pytest 4.0.1, pytest-sugar 0.9.2)
rootdir: /home/martin/Dev/home-assistant, inifile: setup.cfg
plugins: requests-mock-1.5.2, timeout-1.3.3, sugar-0.9.2, rerunfailures-5.0, mock-1.10.0, cov-2.6.0, aiohttp-0.3.0
collecting ... 
 tests/components/esphome/test_config_flow.py ✓✓✓✓                                                                                                                                             100% ██████████

----------- coverage: platform linux, python 3.6.5-final-0 -----------
Name                                              Stmts   Miss  Cover   Missing
-------------------------------------------------------------------------------
homeassistant/components/esphome/config_flow.py      73      4    95%   100, 116-118


Results (0.32s):
       4 passed
Show resolved Hide resolved .coveragerc Outdated
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

I think we're done. Just two possible clean ups left.


@property
def _static_info(self) -> 'SensorInfo':
return super()._static_info

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 16, 2018

Member

This doesn't look necessary.

This comment has been minimized.

@OttoWinter

OttoWinter Dec 16, 2018

Contributor

For the runtime it's not necessary, yes.

The reason it's here is because the parent _static_info property is type annotated as returning EntityInfo. So then my IDE for example highlights the line self._static_info.unit_of_measurement as an error, as EntityInfo doesn't have unit_of_measurement.

This overload tells mypy that _static_info in fact returns a SensorInfo, not just a EntityInfo. This could technically also be done with making EsphomeEntity a class with generic arguments, but I have no idea how to do that without importing the generic type during the process.

As you might have noticed, I like type annotations 😇

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 17, 2018

Member

It's a bit weird if it's not possible to handle this case within type annotations, ie without adding a child property. But I'm fine with it. I also think typing is good, although I haven't started using it so much yet so I don't know all the ins and outs.


@property
def _state(self) -> Optional['SensorState']:
return super()._state

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 16, 2018

Member

See above.

@OttoWinter

This comment has been minimized.

Copy link
Contributor

OttoWinter commented Dec 16, 2018

Also really want to thank you @MartinHjelmare for all your reviews ❤️

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Awesome!

@MartinHjelmare MartinHjelmare merged commit a08bab7 into home-assistant:dev Dec 17, 2018

5 checks passed

Hound 5 violations found.
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 93.029%
Details

@wafflebot wafflebot bot removed the in progress label Dec 17, 2018

@OttoWinter OttoWinter referenced this pull request Dec 17, 2018

Merged

Add native ESPHome API binary sensor #19371

3 of 3 tasks complete

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Add native ESPHome API component (home-assistant#19334)
* Create esphomelib component

* Update requirements

* Remove python 2 string literals

* MVP

* Remove config flow

* Remove config flow translations

* Use dispatcher more

* Use data classes

* Type Hints

* Cleanup on remove

* Use helper and cleanup

* Fix HA stop listener cleanup

* Add config flow

* Fix SyntaxError for Py3.5

* Update

* Lint

* Re-do tests

*  Rename to ESPHome

* Better error message for resolve errors

* Fix tests when aioesphomeapi not installed

* Refactor mock

* Update requirements

* Add strings.json

* 🍵 100% config flow  test coverage

@OttoWinter OttoWinter deleted the OttoWinter:esphomelib branch Jan 5, 2019

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment