jjjonesjr33 / petlibro

Petlibro | Home Assistant integration
https://community.home-assistant.io/t/petlibro-cloud-integration-non-tuya-wip/759978
GNU General Public License v3.0
6 stars 5 forks source link

[Testing]: C4-Dimitri | Battery Device Class & State Class #6

Closed jjjonesjr33 closed 6 days ago

jjjonesjr33 commented 6 days ago

Hey @C4-Dimitri I was looking at the code you were testing out - https://github.com/jjjonesjr33/petlibro/compare/master...C4-Dimitri:petlibro:master I like the idea of switching it to a float, but there was a potential issue that was flagged when going that route:

If "batteryState" is missing or returns something other than a number (such as "unknown"), this will raise a ValueError because a string like "unknown" cannot be converted to a float.

You might be able to handle this scenario safely, by checking whether the "batteryState" is a valid number before casting it to a float. For example:

@property
def battery_state(self) -> float:
    battery_state_value = self._data.get("realInfo", {}).get("batteryState", None)
    try:
        return float(battery_state_value) if battery_state_value is not None else 0.0
    except (ValueError, TypeError):
        return 0.0  # or whatever default makes sense for your case

This approach ensures:

If "batteryState" is present and can be converted to a float, it will be cast correctly.
If it's missing, invalid, or can't be converted, you return a default value (0.0 in this case).

With the way the API pulls information on the second go around with the realID api, this might cause a problem. Let me know how it's working for you or if you need help πŸ˜„

C4-Dimitri commented 6 days ago

You snooping on me!? ;) Currently it works fine with float, i am still trying to get it to report nicely as a battery sensor though, currently its not getting the percent assigned.

But yes, will 100% need some error handling for non float values. We could potentially pull it as a string and convert to float later and validate it then converts nicely.

jjjonesjr33 commented 6 days ago

Haha, I was just looking and seen the new commits 🀣 It looking good with what you're doing!

P.s

I got some more devices added today so if we go this route we'll just need to update them as well πŸ˜„

C4-Dimitri commented 6 days ago

Im going to try to be neater in the future and dev on a branch, just so we will have 1 clean commit at the end when i do a pull request.

But this one will likely be a bit messy till i get it all working, as i like to dev a bit at a time and test frequently.

jjjonesjr33 commented 6 days ago

It's all good, I'm pretty sure I can squash them into a single commit when your ready to do a push/pull. So no worries there πŸ‘πŸ»

I probably should do a dev branch or something, but what I've been doing to on my side is running them in my test environment with Home Assistant and just the Petlibro add-on and then doing live edits and restarts to get things working πŸ˜ƒ especially when I like to break things down and figure out how to fix it or upgrade it.