-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Add IoTMeter integration #120087
base: dev
Are you sure you want to change the base?
Add IoTMeter integration #120087
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing. Please take a look.
There's some guidance on the HA developer page for how to set up a HA dev environment. That way, you don't need to wait for GitHub CI to pass and can fix most/all errors locally.
Please also edit your PR description to add some further information about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick look, icons belong in https://github.com/home-assistant/brands/ and we limit new integrations to one platform (sensor, number, ..) at first to make the review easier. Other platforms can be added in subsequent PRs.
Not all comments are resolved, please resolve them before marking ready for review again |
Could you please tell me what else needs to be done? Based on the comment, I may have corrected everything. |
I thought that after deleting it from the list, the platform would not initialize and that this would be sufficient. To fix this, do I need to delete the |
Yes, we don't want loose pieces of code |
Thanks for the advice. I have made the changes; should I update the branch? |
"dependencies": [], | ||
"documentation": "https://www.home-assistant.io/integrations/iotmeter", | ||
"iot_class": "local_polling", | ||
"requirements": ["aiohttp==3.9.5"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to connect to the device should be encapsulated in a package published on PyPi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to connect to the device should be encapsulated in a package published on PyPi.
Can I simply remove the requirements from manifest.json and use the aiohttp==3.10.0b1
version that is already a dependency in Home Assistant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you need to create your own library with all the calls to the device/service and publish that to Pypi and use that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure you are using a development environment to develop the integration, there are things in here that should have been caught by the pre-commit rules
"I'm sorry, I forgot to include the modified tests that were causing the error." |
Who is vase-uzivatelske-jmeno? |
"My mistake, it should be @lipic. I will fix it." |
_LOGGER = logging.getLogger(__name__) | ||
_LOGGER.setLevel(logging.DEBUG) | ||
|
||
PLATFORMS = [Platform.SENSOR] # Platform.NUMBER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the comment
|
||
async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: | ||
"""Set up IoTMeter from a config entry.""" | ||
_LOGGER.debug("Setting up IoTMeter integration") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
if DOMAIN not in hass.data: | ||
hass.data[DOMAIN] = {} # Ensure that the domain is a dictionary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use entry.runtime_data. Check airgradient for an example. You should extend the ConfigEntry type with the type for the runtime_data.
ip_address = entry.data.get("ip_address") | ||
port = entry.data.get("port", 8000) # Default to port 8000 if not set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use constants
self.total_power: float = 0 | ||
|
||
@property | ||
def state(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use state
p1 = ( | ||
self.coordinator.data.get("P1") - 65535 | ||
if self.coordinator.data.get("P1") > 32767 | ||
else self.coordinator.data.get("P1") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is device specific, please move to the library
if p1 is not None and p2 is not None and p3 is not None: | ||
self.total_power = (float(p1) + float(p2) + float(p3)) / 1000 | ||
return round(self.total_power, 2) | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an abstraction, we want to provide the device to HA as raw as possible
return None | ||
|
||
@property | ||
def extra_state_attributes(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add extra state attributes
@pytest.fixture | ||
def mock_config_flow(hass): | ||
"""Fixture for creating a config flow instance.""" | ||
flow = EVConfigFlow() | ||
flow.hass = hass | ||
return flow | ||
|
||
|
||
@pytest.fixture | ||
async def hass(tmpdir): | ||
"""Fixture for Home Assistant instance.""" | ||
hass = HomeAssistant(tmpdir) | ||
hass.config_entries = config_entries.ConfigEntries( | ||
hass, {} | ||
) # Initialize config_entries with an empty config | ||
|
||
await hass.async_start() | ||
await hass.async_block_till_done() | ||
hass.data["integrations"] = {} # Initialize the 'integrations' key | ||
|
||
yield hass | ||
await hass.async_stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Euh, why? Please check other integrations for how they do their config flow tests
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: