tomaae / homeassistant-truenas

TrueNAS integration for Home Assistant
Apache License 2.0
183 stars 17 forks source link

Replace truenas controller to HA Coordinator #73

Closed cyr-ius closed 1 year ago

cyr-ius commented 1 year ago

Proposed change

I removed the PR#71 which was merged and I submit this draft which contains only the addition of the HA Coordinator More info : https://developers.home-assistant.io/docs/integration_fetching_data/#polling-api-endpoints

the coordinator does exactly what the controller does but it's just a native class of HA

Fix issue #48

Type of change

Checklist

tomaae commented 1 year ago

I will need some time to review this, as its a lot. It is major part shared between all my integrations. Bit busy right now, but I will find the time soon. But I dont like that you removed function comments, those make code much more readable and easier to navigate. At least in pycharm.

cyr-ius commented 1 year ago

I will need some time to review this, as its a lot. It is major part shared between all my integrations. Bit busy right now, but I will find the time soon. But I dont like that you removed function comments, those make code much more readable and easier to navigate. At least in pycharm.

Sorry, I'm using vscode and since the functions all have a description, I thought this would make it easier to read.

I haven't changed the principle of the code, the controller and the coordinator do the same thing. The coordinator is just a more integrated way to HA. I looked at a lot of code present in the HA repository to keep the way of labeling the names

Goo day.

cyr-ius commented 1 year ago

on the "_types" pages, I simply converted the dicts to tuple because the keys were not used and replaced the empty strings with None values

I then merged the model_async_setup_entry and model_update_items functions

I called this merge: async_add_entities which I positioned in the coordinator.py file instead of the model.py file

If this is OK for you, we could later rename model.py to entity.py as other devs do in HA. As well as the parent class is well identified in its file.

The new async_add_entites function practically takes your code, I simply use the platform object which contains all the elements of the current platform to avoid passing many parameters.

github-actions[bot] commented 1 year ago

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

tomaae commented 1 year ago

sorry I'm taking this long. IRL got super busy for me

tomaae commented 1 year ago

I'm still unsure about several things in new system, but I will test it in detail once its ready. By that I dont mean mostly new underlying HA code. Overall I think this is a really good change.

github-actions[bot] commented 1 year ago

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.