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

EnergyID integration #95006

Closed
wants to merge 16 commits into from
Closed

Conversation

JrtPec
Copy link

@JrtPec JrtPec commented Jun 21, 2023

Proposed change

Introducing the EnergyID integration! EnergyID is an online energy monitoring platform. This integration allows you send sensors from within Home Assistant (eg. energy meters, solar panels, batteries, ...) to your EnergyID-record, using their webhooks.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant

This comment was marked as off-topic.

@home-assistant

This comment was marked as off-topic.

1 similar comment
@home-assistant

This comment was marked as off-topic.

@home-assistant

This comment was marked as off-topic.

@JrtPec JrtPec marked this pull request as ready for review June 22, 2023 16:35
@JrtPec JrtPec marked this pull request as draft June 23, 2023 09:16
@JrtPec JrtPec marked this pull request as ready for review June 23, 2023 10:46
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback to get the process started. I haven't worked with webhooks yet, so I will leave the feedback for that open for someone else to pick up

Comment on lines +1 to +5
{
"domain": "energyid",
"name": "EnergyID",
"integrations": ["energyid"]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A brand is only created if there are more integrations under 1 company brand. For example Google has everything from Google Agenda to YouTube and Philips has Philips hue and Philips TV. So this integration is not a brand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. So this json file can simply be removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not removed btw

homeassistant/components/energyid/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/energyid/manifest.json Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft July 27, 2023 23:00
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@JrtPec JrtPec marked this pull request as ready for review October 10, 2023 14:30
@@ -342,6 +342,7 @@ build.json @home-assistant/supervisor
/tests/components/emulated_kasa/ @kbickar
/homeassistant/components/energy/ @home-assistant/core
/tests/components/energy/ @home-assistant/core
/homeassistant/components/energyid/ @JrtPec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the tests should also be added to your name. I think hassfest might be having a bad day with it (I also saw another case of this recently, so maybe its something we have to fix in hassfest)

@@ -342,6 +342,7 @@ build.json @home-assistant/supervisor
/tests/components/emulated_kasa/ @kbickar
/homeassistant/components/energy/ @home-assistant/core
/tests/components/energy/ @home-assistant/core
/homeassistant/components/energyid/ @JrtPec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/homeassistant/components/energyid/ @JrtPec
/homeassistant/components/energyid/ @JrtPec
/tests/components/energyid/ @JrtPec

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 remark and 1 suggestion. Reading the code this time actually make me realise what the integration does. I will ask some other members if this approach is the way to go or if there are any reasons it should be different

await dispatcher.client.get_policy()
except aiohttp.ClientResponseError as error:
_LOGGER.error("Could not validate webhook client")
raise ConfigEntryAuthFailed from error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigEntryAuthFailed should only be raised when the integration has a reauth flow and you dont have one yet. So please raise ConfigEntryError instead


async def async_handle_state_change(self, event: Event) -> bool:
"""Handle a state change."""
await self._upload_lock.acquire()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it maybe work if you did:

async with self._upload_lock:

This way you don't have to manually acquire and release locks. (and if you dislike the extra indent, you could move this function to _async_handle_state_change and call it like

async def async_handle_state_change() -> bool:
    async with self._upload_lock:
        await self._async_handle_state_change()

@home-assistant home-assistant bot marked this pull request as draft November 15, 2023 09:08
@joostlek
Copy link
Member

Oh and sorry for the wait

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jan 14, 2024
@github-actions github-actions bot closed this Jan 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants