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 config flow to linky #26076

Merged
merged 14 commits into from
Sep 4, 2019
Merged

Conversation

Quentame
Copy link
Member

@Quentame Quentame commented Aug 19, 2019

Breaking Change:

Linky platform moved to integration.

configuration.yaml before :

sensor:
  - platform: linky
    username: __email__
    password: __password__

After :

linky:
  - username: __email__
    password: __password__

Documentation updated : home-assistant/home-assistant.io#10284

Description:

Hi !

Just adding Linky a config flow, but I need some help for 3 things.

I am still learning Python with you guys, while coding for HA ... everyone is pleased to make a review, go for it ! (with cool links if it's something tricky, that would be nice 👍)

I made _LOGGER.error to see it better, screenshots will illustrate 😉 with the actual code.

1 : Make the component actually retrieving the data without stop HA boot

I mean, it's the same working retrieve data code, but the async and timeout part makes it not working I think.

So I got a RuntimeError: cannot be called from within the event loop caused by run_callback_threadsafe call in a async method, then the sensors aren't created.

Actually HA is booting because the sensors try to be added after HA starts with a listener and async_start callback, but without it's not, and no logs are coming.

See the logs, at HA starts (the only way the retrieve the data for now)
Capture d’écran 2019-08-19 à 13 44 14

This first point should be though with point 2 in mind ⬇️

  • fixed

2 : Add the Linky sensors directly after adding the integration by the user in the config flow (not only at boot)

I know that it should be added in async_setup_entry but if I do so, it is gonna block HA start and Linky's data are never gonna be retrieved. That's why there is an async_start.

So I get those logs right after adding my integration :
Capture d’écran 2019-08-19 à 13 45 46

But no sensors.

  • fixed

3 : Here it's more a question but it will permits much less duplicated code

Can/Should I display "computed" errors to the user ? (one's that are not in the strings.json file)

I'am telling that because the LinkyClient can raise multiple functional errors, but they got all the same type PyLinkyError.

PyLinkyErroris raised for (quoting the pylinky lib) :

  1. raise PyLinkyError("Can not submit login form")
  2. raise PyLinkyError("Login error: Please check your username/password.")
  3. raise PyLinkyError("Could not access enedis.fr: " + str(e))
  4. raise PyLinkyError("No data")
  5. raise PyLinkyError("Site in maintenance")
  6. raise PyLinkyError("Impossible to decode response: " + str(e) + "\nResponse was: " + str(raw_res.text))
  7. raise PyLinkyError("Enedis.fr answered with an error: " + str(json_output))

So how should I tell the user he has a wrong password ? or if the website is in maintenance and retry later ?

I come up this 3 ideas :

  1. Display the message as it is (but meaning no translation)
  2. Parse the message (no future proof at all)
  3. Change the lib to raise specific Exceptions, wait until the approval and release, update the lib and add specific translations for every exceptions (might be very long)
  4. Just say the user to verify its credentials and if the website isn't in maintenance and retry (all in one but not precise and accurate for the user)
  • fixed

@MartinHjelmare, if you have some time I will appreciate.

Thanks a lot !

PS : config flow inspired with AdGuard Home (Config flow file, by @frenck) and Cert Expiry (still in PR #25624 by @Cereal2nd) code.


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

Not yet

@Quentame
Copy link
Member Author

I actually get the same issue to make iCloud a config flow.

The integration is added to HA, then, when adding all the platforms, the device_tracker calculate the interval with the current zone and other zone distances, which uses run_callback_threadsafe.

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.

Blocking I/O in coroutines, in the event loop, will block the whole home assistant app.

homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
@Quentame
Copy link
Member Author

Ok, now the sensors are added

Capture d’écran 2019-08-25 à 23 51 07

Exemple

So the integration is like
Capture d’écran 2019-08-25 à 23 51 55

But the integration is saying there is not devices (platforms), and it should display the sensors, working on it.
Capture d’écran 2019-08-25 à 23 52 22

I should use ConfigEntry, right ?

@MartinHjelmare
Copy link
Member

Devices will show for the config entry if we implement device_info property for the entities. Please do that in a separate PR later.

https://developers.home-assistant.io/docs/en/device_registry_index.html

homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/linky/sensor.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title Linky: setup ConfigFlow [NEED HELP] Add config flow to linky Aug 26, 2019
@Quentame
Copy link
Member Author

How can I see that's wrong with my pipeline ?

Can't find it.

Capture d’écran 2019-08-27 à 13 51 18

@MartinHjelmare
Copy link
Member

Just run black from the project root and commit the changes:

black --fast homeassistant tests

@Quentame
Copy link
Member Author

Quentame commented Aug 28, 2019

Response to : #26076 (comment)

black --fast homeassistant tests isn't working on my machine (MacOS 10.14.6) or venv.

I get a "command not found error" on both.

Nothing helped in Set up Development Environment and Testing your code of the developer documentation.

@MartinHjelmare
Copy link
Member

Install black.

https://pypi.org/project/black/

@Quentame
Copy link
Member Author

Yep, naturally ... found it odd to be a beta

@Quentame
Copy link
Member Author

Quentame commented Aug 28, 2019

pyLinky 0.4.0 just released with my PR into it, that was fast !
It's gonna be better to handle user feedbacks.

https://github.com/Pirionfr/pyLinky/releases/tag/v0.4.0

@Quentame
Copy link
Member Author

I've fixed all reviews, so what's next ? Clicking on "Ready for review" button in order to be merged or open a new PR ?

For me, it misses 2 PRs : add device_info to add devices for the config flow integration + translate the component to French (since the service is only available in France).

.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/linky/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/linky/config_flow.py Outdated Show resolved Hide resolved
@MartinHjelmare
Copy link
Member

Don't open a new PR. Just click ready for review.

@Quentame Quentame marked this pull request as ready for review August 29, 2019 11:54
@Quentame
Copy link
Member Author

Quentame commented Aug 29, 2019

Tested with :

  • config flow good credentials
  • config flow fake credentials
  • config flow Enedis technical issue
  • config flow existing username
  • import unique username
  • import Enedis technical issue
  • import existing username
  • unit test

Component documentation needs to be updated

@Quentame
Copy link
Member Author

Quentame commented Sep 3, 2019

Working on tests ...

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!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

@MartinHjelmare
Copy link
Member

Please don't squash commits after review has started to make it easier for readers to track changes.

@Quentame
Copy link
Member Author

Quentame commented Sep 3, 2019

Ok will do, habits.

When I do tox -e py37 -- tests/components/linky/test_config_flow.py I got :

Capture d’écran 2019-09-03 à 23 04 35

How can I fix this ?

@MartinHjelmare
Copy link
Member

Have you rebuilt the tox environment, -r?

@Quentame
Copy link
Member Author

Quentame commented Sep 3, 2019

tox -r command :

Capture d’écran 2019-09-04 à 00 03 29

@MartinHjelmare
Copy link
Member

There's too little information in that log excerpt for me to say anything about.

@Quentame
Copy link
Member Author

Quentame commented Sep 3, 2019

Ups, wrong image, updated #26076 (comment)

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Sep 3, 2019

Doesn't help, sorry. You can check the tox file logs and see if they say anything more.

@MartinHjelmare
Copy link
Member

Good!

@MartinHjelmare MartinHjelmare merged commit b4058b5 into home-assistant:dev Sep 4, 2019
@Quentame Quentame deleted the linky/config-flow branch September 4, 2019 08:07
@lock lock bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants