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

Bring code more up to current #228

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Bring code more up to current #228

merged 1 commit into from
Jul 18, 2022

Conversation

marcelvriend
Copy link
Contributor

@marcelvriend marcelvriend commented Jun 15, 2022

Proposed changes

  • Use entity descriptions for sensor and binary sensor platform.
  • Replace the custom JSON encoder with the HA default.
  • Remove the broken mechanism to retrieve Grocy data only when it has been modified.
  • Remove unused translation strings. The description with URL is redundant with the help icon and better not to have URLs in language specific files.
  • Set missing unit of measurement for shopping list and stock sensor.
  • Hide entities in HA for disabled features in Grocy.
  • Request for enabled Grocy features only on integration initialization.
  • Rewrite the entity refresh in some service calls to be not entity id specific.
  • Split the coordinator class into a separate file.
  • Clean up code.

No breaking changes.

Out of scope issues

  • With all entities enabled and having a reasonable amount of data, there are a lot of API requests every 30 seconds. Already improved it a bit, but it is still a lot.

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
Copy link
Contributor Author

@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
Copy link
Contributor

ludeeus commented Jun 15, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

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.

custom_components/grocy/__init__.py Outdated Show resolved Hide resolved
custom_components/grocy/entity.py Show resolved Hide resolved
@marcelvriend
Copy link
Contributor Author

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

@marcelvriend marcelvriend marked this pull request as ready for review June 15, 2022 12:06
custom_components/grocy/entity.py Outdated Show resolved Hide resolved
custom_components/grocy/entity.py Outdated Show resolved Hide resolved
custom_components/grocy/binary_sensor.py Outdated Show resolved Hide resolved
@ludeeus
Copy link
Contributor

ludeeus commented Jun 26, 2022

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

@marcelvriend
Copy link
Contributor Author

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

@isabellaalstrom
Copy link
Collaborator

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

@isabellaalstrom isabellaalstrom merged commit 161e482 into custom-components:master Jul 18, 2022
@isabellaalstrom
Copy link
Collaborator

isabellaalstrom commented Jul 18, 2022

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
Copy link
Collaborator

isabellaalstrom commented Jul 18, 2022

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
Copy link
Contributor Author

Thanks! There are no breaking changes indeed.

@marcelvriend marcelvriend deleted the feature/refactor-legacy-code branch July 18, 2022 19:03
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.

None yet

3 participants