SonnenladenGmbH / APsystems-EZ1-API-HomeAssistant

The APsystems EZ1 Integration offers a streamlined interface for interacting with the local API of APsystems EZ1 Microinverters using Home Assistant.
MIT License
77 stars 11 forks source link

Move P1 & P2 informations to seperate sensors and reduce code repitition #4

Closed CM000n closed 7 months ago

CM000n commented 7 months ago

Hello, with reference to my issue https://github.com/SonnenladenGmbH/APsystems-EZ1-API-HomeAssistant/issues/2, in this PR, independent sensors are created from the information of P1 & P2 and these are removed from the attributes.

I have also simplified the code a little because it contained repetitive patterns, especially in the async_update methods of the sensor classes. So I created a common method for updating sensor data in the BaseSensor class to reduce repetition and utilized inheritance to set class attributes in BasePowerSensor and BaseEnergySensor instead of repeating them in each subclass.

I think the code can be further simplified by restructuring and using SensorEntityDescription instead of defining each sensor in it's own subclass. But I have to think more about that first

mawoka-myblock commented 7 months ago

have a look at this commit ;) https://github.com/SonnenladenGmbH/APsystems-EZ1-API-HomeAssistant/commit/f06703e73c57e96909bf5e309c05f427c0141a57

CM000n commented 7 months ago

Please remove the Optional again and use TYPE | None instead

If you really want to and this is the codestyle here. But in my opinion, the Optional specification is much easier to read.

CM000n commented 7 months ago

have a look at this commit ;) f06703e

OK, what was the decision behind this? As I wrote in the issue, it's not really good style for Home Assistant sensors to put things that change frequently in the attributes.

mawoka-myblock commented 7 months ago

have a look at this commit ;) f06703e

OK, what was the decision behind this? As I wrote in the issue, it's not really good style for Home Assistant sensors to put things that change frequently in the attributes.

I've done that, since I haven't read this part in the docs and I thought that this information was pretty niche

CM000n commented 7 months ago

Please remove the Optional again and use TYPE | None instead

Reversed 😉

mawoka-myblock commented 7 months ago

Please remove the Optional again and use TYPE | None instead

Reversed 😉

Thanks! I'm gonna have to test it tomorrow when the inverter is online ;)

mawoka-myblock commented 7 months ago

Sorry, I won't manage checking it out today. I'll try it tomorrow.

lboue commented 7 months ago

@mawoka-myblock Can you merge that PR?

mawoka-myblock commented 7 months ago

When this is resolved, I'll merge the PR. Sorry for the delayed response!

CM000n commented 7 months ago

Please remove all mentions of the attributes.

Done (hopefully) 😃 I'm on the road right now. Otherwise I'll have another look later.

mawoka-myblock commented 7 months ago

Finally (from my side)! Thank you for your patience and work! A new release will be created soon. Thanks! ❤️

CM000n commented 7 months ago

Thx for your helpfull feedback 😊