Skip to content

Bring code more up to current#228

Merged
isabellaalstrom merged 1 commit into
custom-components:masterfrom
marcelvriend:feature/refactor-legacy-code
Jul 18, 2022
Merged

Bring code more up to current#228
isabellaalstrom merged 1 commit into
custom-components:masterfrom
marcelvriend:feature/refactor-legacy-code

Conversation

@marcelvriend

@marcelvriend marcelvriend commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

ludeeus commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.

Comment thread custom_components/grocy/__init__.py Outdated
Comment thread custom_components/grocy/entity.py
@marcelvriend

Copy link
Copy Markdown
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
Comment thread custom_components/grocy/entity.py Outdated
Comment thread custom_components/grocy/entity.py Outdated
Comment thread custom_components/grocy/binary_sensor.py Outdated
@marcelvriend marcelvriend requested a review from ludeeus June 25, 2022 17:13
@ludeeus

ludeeus commented Jun 26, 2022

Copy link
Copy Markdown
Contributor

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

@marcelvriend

Copy link
Copy Markdown
Contributor Author

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

@isabellaalstrom

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

isabellaalstrom commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

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

isabellaalstrom commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

3 participants