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

Refactoring changes #15

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Refactoring changes #15

wants to merge 8 commits into from

Conversation

kubasaw
Copy link

@kubasaw kubasaw commented Mar 29, 2024

This is quite huge PR with a lot of changes (I think, breaking ones):

  • Code refactor: I've rewritten a substantial portion of our integration to streamline the maintenance process. By implementing a clearer hierarchy of class inheritance, we can now manage and extend our code more efficiently.
  • Sensor Enhancements: I've adjusted the DeviceClass for sensors to ensure they display correctly on the Home Assistant frontend. This should provide users with more accurate and informative data representation.
  • Expiration Timestamp Sensor: I've introduced a new sensor that gives a glance, when sensor will expire. For me, it is more informative than looking at application timestamp or its age.
  • Improved Low/High Sensor Functionality: Fixed an issue where Low/High are always set as OK. When I was writing the remaining parts of the code, I had a low sugar level and I was not getting the appropriate sensor state: LibreLink API always returned False for the isHigh and isLow fields 😆

This is just a draft PR. I've got plans for more fixes, especially regarding handling multiple tracked persons. However, I can't say for certain when those updates will be ready due to the limited time I can devote to it.

I'd love to hear your thoughts and feedback.

@gillesvs
Copy link
Owner

This is quite huge PR with a lot of changes (I think, breaking ones):

  • Code refactor: I've rewritten a substantial portion of our integration to streamline the maintenance process. By implementing a clearer hierarchy of class inheritance, we can now manage and extend our code more efficiently.
  • Sensor Enhancements: I've adjusted the DeviceClass for sensors to ensure they display correctly on the Home Assistant frontend. This should provide users with more accurate and informative data representation.
  • Expiration Timestamp Sensor: I've introduced a new sensor that gives a glance, when sensor will expire. For me, it is more informative than looking at application timestamp or its age.
  • Improved Low/High Sensor Functionality: Fixed an issue where Low/High are always set as OK. When I was writing the remaining parts of the code, I had a low sugar level and I was not getting the appropriate sensor state: LibreLink API always returned False for the isHigh and isLow fields 😆

This is just a draft PR. I've got plans for more fixes, especially regarding handling multiple tracked persons. However, I can't say for certain when those updates will be ready due to the limited time I can devote to it.

I'd love to hear your thoughts and feedback.

Hi Kuba,

Thanks for your contribution. I will have a look at it asap.
For Low/high binary sensors, my understanding is that they only activate when you see high or low on the app (which mean under 40 or over 400). I agree it is not very useful as hopefully you do not go to these extremes often.

@gillesvs
Copy link
Owner

Hello @kubasaw ,

You are clearly more advanced than myself (this is my first program ;-)) but I understand than this update will "break" existing sensors.
Don't you think that it is better to wait for your complete refactoring and then propose to the community a major version which will lead to sensor changes and therefore imply regressions which will have to be handled by everyone.
I understand that your code is much cleaner, but some people may think that you do not need to repair what is not broken (:-))
What is your suggestion?

@kubasaw
Copy link
Author

kubasaw commented Apr 1, 2024

Hello,

I've created this PR as a draft, and it probably doesn't make much sense to merge it at the moment. I mainly wanted to signal that I'm working on something, and if needed, my ongoing work can be utilized.

I'll aim to finish the majority of the changes this month. I'm not entirely certain about some aspects, so if I find myself unsure about what to do next, I'll reach out here with my questions.

So if this draft can just hang around here, that would be great. Once it reaches a reasonable level of 'mergeability', I'll also think about how to sensibly migrate previous configurations.

K

@gillesvs
Copy link
Owner

gillesvs commented Apr 2, 2024

Hello,

I've created this PR as a draft, and it probably doesn't make much sense to merge it at the moment. I mainly wanted to signal that I'm working on something, and if needed, my ongoing work can be utilized.

I'll aim to finish the majority of the changes this month. I'm not entirely certain about some aspects, so if I find myself unsure about what to do next, I'll reach out here with my questions.

So if this draft can just hang around here, that would be great. Once it reaches a reasonable level of 'mergeability', I'll also think about how to sensibly migrate previous configurations.

K

Hello, Yes please do not hesitate if you have questions. For the moment, it seems that there is no pb for users.
Thanks again for your refactoring, it helps me understand better ways to structure things.

@xamrex
Copy link

xamrex commented Aug 28, 2024

Sorry for comment here, but Only here I can mention @kubasaw ,
this issue: #27 is not present in Kuba's fork ;-)

@lebherz
Copy link

lebherz commented Dec 11, 2024

@kubasaw: I tested your branch! perfect! everythink is fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants