search

Miicroo / ha-birthdays

Birthday integration for HomeAssistant
24 stars 8 forks source link

Added unique_id #13

Closed Darknetzz closed 9 months ago

Darknetzz commented 1 year ago

This change allows for the presence of "unique_id" field which allows you to edit the entities in the Home Assistant GUI.

Closes #12

Darknetzz commented 1 year ago

FYI, I have never written a custom component for Home Assistant before, and I am by no means an expert. But if you'd like you can take a look at the code.

Miicroo commented 9 months ago

I have tested this in my instance. It is working!

Nice work, and fast too! I added some comments, mainly based around getting rid of the age_at_next_birthday variable and fixing some of the documentation. Really good work though, should be done very soon :)

Darknetzz commented 9 months ago

I think I have made the changes you commented now (including small updates to README).

It's tested on my instance (after removing restored entities again), it's working!

Sorry for being such an absolute noob, like I said I have never worked with HA custom components before, but thank you for being patient with me. 😁

Miicroo commented 9 months ago

Sorry for being such an absolute noob, like I said I have never worked with HA custom components before, but thank you for being patient with me. 😁

I think it looks good for someone who hasn't tried it before, and that is also why it is great to have reviews! :D I learn a lot too, and good to understand others point of view. Added a few formatting comments, but code wise looks good. Will review as soon as I can after the changes

Darknetzz commented 9 months ago

I think it looks good for someone who hasn't tried it before, and that is also why it is great to have reviews! :D I learn a lot too, and good to understand others point of view. Added a few formatting comments, but code wise looks good. Will review as soon as I can after the changes

Spaces removed!

Miicroo commented 9 months ago

Found a few problems when testing, but added suggestions how to fix (and why they showed up). I really should start adding unit tests to these repos to avoid this, but for now we have to do it manually :D

Miicroo commented 9 months ago

Looks good, approved! I realized that there might still be some migration work for old users. Since the previous uinque_id is None and the new unique_id is not None, I think HA will treat these as two separate entities. No worries though, that is a price we have to pay for me being lazy and not implementing this in the beginning, and I will add a note about it in the release notes. :)