custom-components / grocy

Custom Grocy integration for Home Assistant
Apache License 2.0
156 stars 47 forks source link

Bring code more up to current #228

Closed marcelvriend closed 2 years ago

marcelvriend commented 2 years ago

Proposed changes

No breaking changes.

Out of scope issues

The mechanism to prevent this was already broken, so nothing has changed in that regard. Although easy to fix the bug, it still ignores time zones and will likely cause refresh issues for many users. Needs more attention and therefore removed it for now.

marcelvriend commented 2 years ago

@isabellaalstrom, would you like to take a look at it? Also curious what you think about the possible breaking change.

Might do a production release of the current beta, before eventually merging this?

ludeeus commented 2 years ago

For the JSON decoder, inherit the one from Core bin in a custom decoder so we can have no breaking change and still fall back to it

isabellaalstrom commented 2 years ago

If we can do it like @ludeeus says, without breaking change that would be best. Otherwise like you say, a production release of current beta before. I might do that now anyway? I am running beta with no issues.

marcelvriend commented 2 years ago

Yes, will do that. Production release is fine, from the 300 users with analytics enabled 50 are on the beta and I haven't seen any issues.

marcelvriend commented 2 years ago

Commit pushed with a custom JSON encoder that inherits from the HA Core ExtendedJSONEncoder, so no more breaking changes.

ludeeus commented 2 years ago

As I do not use this integration, and further reviews have to be done by @isabellaalstrom

marcelvriend commented 2 years ago

@isabellaalstrom, are you still waiting for something or just haven't had time to review yet?

isabellaalstrom commented 2 years ago

Oh crap I missed this entirely! On vacay right now. Will look at it asap.

isabellaalstrom commented 2 years ago

I looked the code over, and think it looks good, but I'm not all that great with python really (😬) so I'm gonna merge it, release a pre-release and try it out!

isabellaalstrom commented 2 years ago

This should be version 5.0.0, right? Since breaking etc

Edit: Forgot the breaking changes was taken out (?). Maybe continue on v4 then

marcelvriend commented 2 years ago

Thanks! There are no breaking changes indeed.